Bug 28445 - Use the task queue for the batch delete and update items tool
Summary: Use the task queue for the batch delete and update items tool
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi (tcohen)
URL: https://gitlab.com/joubu/Koha/-/commi...
Keywords: Manual
Depends on: 22417 26080 27526 28158 28765 28769
Blocks: 29567 29689 29797 30178 30725 31066 31234 31455 32464 32550 32710 37986
  Show dependency treegraph
 
Reported: 2021-05-25 12:12 UTC by Jonathan Druart
Modified: 2024-09-23 11:40 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This enhancement changes the batch item modification and deletion tools so that they now use the task queue feature (added in Koha 21.05) instead of using background jobs. For the library staff member this provides more information on the progress of the task.
Version(s) released in:
21.11.00
Circulation function:


Attachments
Bug 28445: WIP - Use the task queue for the batch delete and update items tool (38.24 KB, patch)
2021-05-25 12:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Isolate add item form to an include file (16.78 KB, patch)
2021-07-27 16:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Move controller code to Koha::UI::Form::Builder::Item (35.50 KB, patch)
2021-07-27 16:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Use Koha::UI::Form::Builder::Item from batchmod as well (27.30 KB, patch)
2021-07-27 16:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Adjust code to handle regexs (20.86 KB, patch)
2021-07-27 16:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Start form with empty library for batch mod (3.55 KB, patch)
2021-07-27 16:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Add the capability to provide more info to the background job detail view (4.94 KB, patch)
2021-07-27 16:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Use the task queue for the batch delete and update items tool (153.67 KB, patch)
2021-07-27 16:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: UI change for repeatable and regex (2.36 KB, patch)
2021-07-27 16:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Fix encoding issue with the background job's report (1.20 KB, patch)
2021-07-27 16:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Fix encoding issue with the background job's report (1.98 KB, patch)
2021-07-28 07:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Don't display the items if not finished yet (1.14 KB, patch)
2021-07-28 07:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Isolate add item form to an include file (16.78 KB, patch)
2021-10-06 13:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Move controller code to Koha::UI::Form::Builder::Item (35.63 KB, patch)
2021-10-06 13:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Use Koha::UI::Form::Builder::Item from batchmod as well (27.30 KB, patch)
2021-10-06 13:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Adjust code to handle regexs (20.86 KB, patch)
2021-10-06 13:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Start form with empty library for batch mod (3.55 KB, patch)
2021-10-06 13:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Use the task queue for the batch delete and update items tool (151.57 KB, patch)
2021-10-06 13:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: UI change for repeatable and regex (2.36 KB, patch)
2021-10-06 13:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Fix encoding issue with the background job's report (1.98 KB, patch)
2021-10-06 13:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Don't display the items if not finished yet (1.14 KB, patch)
2021-10-06 13:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Improve robustness of tests (2.90 KB, patch)
2021-10-06 13:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Improve robustness of tests (1.10 KB, patch)
2021-10-07 09:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Fix subfields to blank (2.08 KB, patch)
2021-10-11 14:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Don't surround the whole batch with a transaction (10.29 KB, patch)
2021-10-11 14:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Use the task queue for the batch delete and update items tool (151.57 KB, patch)
2021-10-13 06:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: UI change for repeatable and regex (2.36 KB, patch)
2021-10-13 06:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Fix encoding issue with the background job's report (1.98 KB, patch)
2021-10-13 06:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Don't display the items if not finished yet (1.14 KB, patch)
2021-10-13 06:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Improve robustness of tests (1.10 KB, patch)
2021-10-13 06:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Fix subfields to blank (2.08 KB, patch)
2021-10-13 06:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Don't surround the whole batch with a transaction (10.29 KB, patch)
2021-10-13 06:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Don't surround the whole batch with a transaction (10.04 KB, patch)
2021-10-13 15:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: delete_biblios is a boolean (1.08 KB, patch)
2021-10-13 15:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Respect MaxItemsToDisplayForBatchMod from job detail view (2.52 KB, patch)
2021-10-13 15:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Split and join on ' | ' instead of '|' for repeatable (2.89 KB, patch)
2021-10-13 15:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Adjust code to handle regexs (20.65 KB, patch)
2021-10-15 14:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Start form with empty library for batch mod (3.55 KB, patch)
2021-10-15 14:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Use the task queue for the batch delete and update items tool (151.37 KB, patch)
2021-10-15 14:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: UI change for repeatable and regex (2.36 KB, patch)
2021-10-15 14:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Fix encoding issue with the background job's report (1.98 KB, patch)
2021-10-15 14:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Don't display the items if not finished yet (1.14 KB, patch)
2021-10-15 14:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Improve robustness of tests (1.10 KB, patch)
2021-10-15 14:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Fix subfields to blank (2.08 KB, patch)
2021-10-15 14:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Don't surround the whole batch with a transaction (10.04 KB, patch)
2021-10-15 14:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: delete_biblios is a boolean (1.08 KB, patch)
2021-10-15 14:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Respect MaxItemsToDisplayForBatchMod from job detail view (2.52 KB, patch)
2021-10-15 14:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Split and join on ' | ' instead of '|' for repeatable (2.89 KB, patch)
2021-10-15 14:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Isolate add item form to an include file (16.83 KB, patch)
2021-10-15 16:41 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 28445: Move controller code to Koha::UI::Form::Builder::Item (35.68 KB, patch)
2021-10-15 16:41 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 28445: Use Koha::UI::Form::Builder::Item from batchmod as well (27.35 KB, patch)
2021-10-15 16:41 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 28445: Adjust code to handle regexs (20.70 KB, patch)
2021-10-15 16:41 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 28445: Start form with empty library for batch mod (3.61 KB, patch)
2021-10-15 16:41 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 28445: Use the task queue for the batch delete and update items tool (151.38 KB, patch)
2021-10-15 16:41 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 28445: UI change for repeatable and regex (2.41 KB, patch)
2021-10-15 16:42 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 28445: Fix encoding issue with the background job's report (2.04 KB, patch)
2021-10-15 16:42 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 28445: Don't display the items if not finished yet (1.20 KB, patch)
2021-10-15 16:42 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 28445: Improve robustness of tests (1.16 KB, patch)
2021-10-15 16:42 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 28445: Fix subfields to blank (2.13 KB, patch)
2021-10-15 16:42 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 28445: Don't surround the whole batch with a transaction (10.09 KB, patch)
2021-10-15 16:42 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 28445: delete_biblios is a boolean (1.13 KB, patch)
2021-10-15 16:42 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 28445: Respect MaxItemsToDisplayForBatchMod from job detail view (2.58 KB, patch)
2021-10-15 16:42 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 28445: Split and join on ' | ' instead of '|' for repeatable (2.94 KB, patch)
2021-10-15 16:42 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 28445: (follow-up) Display count of modified items when too many to display (1.43 KB, patch)
2021-10-15 16:42 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 28445: Isolate add item form to an include file (16.89 KB, patch)
2021-10-15 18:50 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 28445: Move controller code to Koha::UI::Form::Builder::Item (35.73 KB, patch)
2021-10-15 18:50 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 28445: Use Koha::UI::Form::Builder::Item from batchmod as well (27.40 KB, patch)
2021-10-15 18:50 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 28445: Adjust code to handle regexs (20.75 KB, patch)
2021-10-15 18:50 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 28445: Start form with empty library for batch mod (3.66 KB, patch)
2021-10-15 18:50 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 28445: Use the task queue for the batch delete and update items tool (151.43 KB, patch)
2021-10-15 18:50 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 28445: UI change for repeatable and regex (2.46 KB, patch)
2021-10-15 18:50 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 28445: Fix encoding issue with the background job's report (2.09 KB, patch)
2021-10-15 18:50 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 28445: Don't display the items if not finished yet (1.25 KB, patch)
2021-10-15 18:50 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 28445: Improve robustness of tests (1.21 KB, patch)
2021-10-15 18:50 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 28445: Fix subfields to blank (2.19 KB, patch)
2021-10-15 18:50 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 28445: Don't surround the whole batch with a transaction (10.14 KB, patch)
2021-10-15 18:51 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 28445: delete_biblios is a boolean (1.18 KB, patch)
2021-10-15 18:51 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 28445: Respect MaxItemsToDisplayForBatchMod from job detail view (2.63 KB, patch)
2021-10-15 18:51 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 28445: Split and join on ' | ' instead of '|' for repeatable (3.00 KB, patch)
2021-10-15 18:51 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 28445: (follow-up) Display count of modified items when too many to display (1.48 KB, patch)
2021-10-15 18:51 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 28445: Add tests for subfields_to_prefill (3.89 KB, patch)
2021-10-28 08:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Correctly count modified items (1.01 KB, patch)
2021-10-28 10:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Remove modified_itemitemnumbers (765 bytes, patch)
2021-10-28 10:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Fix LostItem call and display errors (1.38 KB, patch)
2021-10-28 10:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Add diag for failing test (1.79 KB, patch)
2021-11-25 12:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: (follow-up) Add diag for failing test (3.80 KB, patch)
2021-11-25 12:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28445: Prevent UI/Form/Builder/Item.t to fail randomly (2.93 KB, patch)
2021-11-25 12:56 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2021-05-25 12:12:56 UTC

    
Comment 1 Jonathan Druart 2021-05-25 12:15:05 UTC
Created attachment 121372 [details] [review]
Bug 28445: WIP - Use the task queue for the batch delete and update items tool
Comment 2 Jonathan Druart 2021-07-27 16:40:04 UTC
Created attachment 123221 [details] [review]
Bug 28445: Isolate add item form to an include file

To ease reusability
Comment 3 Jonathan Druart 2021-07-27 16:40:08 UTC
Created attachment 123222 [details] [review]
Bug 28445: Move controller code to Koha::UI::Form::Builder::Item

To ease reusability
Comment 4 Jonathan Druart 2021-07-27 16:40:13 UTC
Created attachment 123223 [details] [review]
Bug 28445: Use Koha::UI::Form::Builder::Item from batchmod as well

Now that it's reusable, let use it somewhere else!
Comment 5 Jonathan Druart 2021-07-27 16:40:17 UTC
Created attachment 123224 [details] [review]
Bug 28445: Adjust code to handle regexs
Comment 6 Jonathan Druart 2021-07-27 16:40:22 UTC
Created attachment 123225 [details] [review]
Bug 28445: Start form with empty library for batch mod
Comment 7 Jonathan Druart 2021-07-27 16:40:26 UTC
Created attachment 123226 [details] [review]
Bug 28445: Add the capability to provide more info to the background job detail view

We already had the need for that, when bibliographic records are
modified in batch we wanted to add a "Add to list" feature, and so pass
a list of lists/virtual shelves to the template.

Here we will want to pass the infos of the items that have been modified
to display a table.
Comment 8 Jonathan Druart 2021-07-27 16:40:31 UTC
Created attachment 123227 [details] [review]
Bug 28445: Use the task queue for the batch delete and update items tool

Here we go!

Disclaimer: this patch is huge and does many things, but splitting it in
several chunks would be time consuming and painful to rebase. However it
adds many tests and isolate/refactor code to make it way more reusable.

This patchset will make the "batch item modification" and "batch item
deletion" features use the task queue (reminder: Since bug 28158, and so
21.05.00, we do no longer use the old "background job" functionality and
the user does not get any info about the progress of the job).

More than that, more of the code to build an item form and a list of
items is now isolated in module (.pm) and include files (.inc)

We are reusing the changes made by bug 27526 that simplifies the way we
edit/create items (no more unecessary serialization Koha > MARC > MARCXML
> XML > HTML)

New module:
* Koha::BackgroundJob::BatchDeleteItem
    Subclass for process item deletion in batch
* Koha::BackgroundJob::BatchUpdateItem
    Subclass for process item modification in batch
* Koha::Item::Attributes
    We needed an object to represent item's attributes that are not
    mapped with a koha field (aka "more subfields xml")
    This module will help us to create the marcxml from a hashref and the
    reverse.
* Koha::UI::Form::Builder::Item
    The code that was used to build the add/edit item form is
    centralised in this module. In conjunction with the
    subfields_for_item BLOCK (from html_helpers.inc) it will be really
    easy to reuse this code in other places where the item form is used
    (acquisition and serials modules)
* Koha::UI::Table::Builder::Items
    Same as previously for the table. We are now using this table from 3
    different places (batch item mod, batch item del, backgroung job
    detail view) and the code is only in one place.
    To use with items_table_batchmod BLOCK (still from html_helpers.inc)

This patch is fixing some bugs about repeatable subfields and regex. A UI
change will reflect the limitation: if you want to apply a regex on a
subfield you cannot add several subfields for the same subfield code.

Test plan:

Prepare the ground:
- Make sure you are always using a bibliographic/item record using the framework
you are modifying!
- Add some subfields for items that are not mapped with a koha field
(note that you can use 'é' for more fun, don't try more funny
characters)
- Make some subfields (mapped and not mapped with a kohafield)
repeatable
- Add default values to some of your subfields

There are 4 main screens to test:
1. Add/edit item form
The behaviour should be the same before and after this patch.
See test plan from bug 27526.
Those 2 prefs must be tested:
    * SubfieldsToAllowForRestrictedEditing
    * SubfieldsToUseWhenPrefill

2. Batch modification
a. Fill some values, play with repeatable and regex.
Note that the behaviour in master was buggy, only the first value was modified by the regex:
    * With subfield = "a | b"
    1 value added with "new"
    => "new | b"

    * With subfield = "a | b"
    2 new fields "new1","new2"
    => "new2 | b"

Important note: For repeatable subfields, a regex will apply on the subfields in
the "concatenated form". To apply the regex on all the different subfields of a given
subfield code you must use the "g" modifier.
This could be improved later, but keep in mind that it's not a regression or behaviour
change.

b. Play with the "Populate fields with default values from default framework" checkbox

c. Use this tool to modify items and play with the different sysprefs that
interfer with it:
    * NewItemsDefaultLocation
    * SubfieldsToAllowForRestrictedBatchmod
    * MaxItemsToDisplayForBatchMod
    * MaxItemsToProcessForBatchMod

3. Batch deletion
a. Batch delete some items
b. Check items out and try to delete them
c. Use the "Delete records if no items remain" checkbox to delete
bibliographic records without remaining items.
d. Play with the following sysprefs and confirm that it works as
expected:
    * MaxItemsToDisplayForBatchDel
e. Stress the tool: Go to the confirmation screen with items that can be
deleted, don't request the job to be processed right away, but check the
item out before.

4. Background job detail view
You must have seen it already if you are curious and tested the above.
When a new modification or deletion batch is requested, the confirmation
screen will tell you that the job has enqueued. A link to the progress
of the job can be followed.
On this screen you will be able to see the result of the job once it's
fully processed.

QA notes:
* There are some FIXME's that are not blocker in my opinion. Feel free to
discuss them if you have suggestions.
* Do we still need MaxItemsToProcessForBatchMod?
* Prior to this patchset we had a "Return to the cataloging module" link
if we went from the cataloguing module and that the biblio was deleted.
We cannot longer know if the biblio will be deleted but we could display
a "Go to the cataloging module" link on the "job has been enqueued"
screen regardless from where we were coming from.
Comment 9 Jonathan Druart 2021-07-27 16:40:36 UTC
Created attachment 123228 [details] [review]
Bug 28445: UI change for repeatable and regex

This patch applies the changes describe in the main commit message about
the "limitation" and "the behaviour in master was buggy".
Comment 10 Jonathan Druart 2021-07-27 16:40:40 UTC
Created attachment 123229 [details] [review]
Bug 28445: Fix encoding issue with the background job's report
Comment 11 Jonathan Druart 2021-07-27 16:42:04 UTC
Remote branch, with all the dependencies applied, available for testing: https://gitlab.com/joubu/Koha/-/commits/bug_28445
Comment 12 Jonathan Druart 2021-07-28 07:48:01 UTC
Created attachment 123239 [details] [review]
Bug 28445: Fix encoding issue with the background job's report
Comment 13 Jonathan Druart 2021-07-28 07:48:05 UTC
Created attachment 123240 [details] [review]
Bug 28445: Don't display the items if not finished yet
Comment 14 Jonathan Druart 2021-07-28 12:44:40 UTC
> (note that you can use 'é' for more fun, don't try more funny characters)

This is not correct, any 4-byte character is supposed to work.
Comment 15 David Cook 2021-07-29 04:28:53 UTC
(In reply to Jonathan Druart from comment #14)
> > (note that you can use 'é' for more fun, don't try more funny characters)
> 
> This is not correct, any 4-byte character is supposed to work.

Should that be 1-4 byte character since UTF-8 characters are all between 1-4 bytes long?

(J'adore couper les cheveux en quatre... ﷐[U+1F605]﷑)
Comment 16 Jonathan Druart 2021-07-29 06:40:16 UTC
(In reply to David Cook from comment #15)
> (In reply to Jonathan Druart from comment #14)
> > > (note that you can use 'é' for more fun, don't try more funny characters)
> > 
> > This is not correct, any 4-byte character is supposed to work.
> 
> Should that be 1-4 byte character since UTF-8 characters are all between 1-4
> bytes long?

Oui mais, "qui peut le plus peut le moins!" (who can do more, can do less)
Comment 17 Kyle M Hall (khall) 2021-09-02 13:10:12 UTC
I'm a bit concerned by the way this patch set is handling the UI. A UI form builder module feels pretty regressive. I'm even seeing CSS embedded in a Perl module, which feels like it should be a hard no.

What I'm seeing here is re-enforces the problem that we are coming up on in Koha over and over. Scalability. For 20 years Koha has been processing all data monolithically on the back end, and then presenting it to the user on page load. This worked fine when Koha was being used by small libraries. Now, Koha is being adopted by huge library system, and those designs that were great in the past are showing their age. Time and time again we find areas of Koha where a page simply cannot load because a library has so much data.

That is why we as a community chose to implement a REST API. This is what allows us to make pages load quickly and dynamically using DataTables and other tools.

I guess my question is, why? Why add a Perl module that's so tightly integrated with html generation that it has CSS embedded in it's code? Why not use the API? Why not make smarter objects and make cleaner TT templates?
Comment 18 Jonathan Druart 2021-09-02 16:13:29 UTC
Thanks for your input.
These patches are more from perfect, I agree. But the idea is to improve the current situation. And I think this move goes toward the following goals: isolate the code, centralise it, make it more robust by providing tests.
These item form and table are used in several places over the code, and having this code in a module makes it reusable easily.
After this move the idea is to reuse it where this code is duplicated.
Once everything will use the same code it will be easier to move to a better solution.
Comment 19 Jonathan Druart 2021-09-02 17:39:57 UTC
(In reply to Jonathan Druart from comment #18)
> These patches are more from perfect, I agree.

*Far* from perfect! :D
Comment 20 Jonathan Druart 2021-09-09 09:54:15 UTC
As I didn't get a reply I will try to provide a more complete, and maybe more convincing, answer.

I would like to highlight that those patches are not adding *new* code to Koha::UI. The code there is moved from current code that is duplicated in several places: additem and batchmod so far (for the add item form and the items table).

To make the item batchmod tool use the task queue I had to reuse the same code from batchmod.pl, and instead of reusing the code as it, I preferred to make it more robust, and reusable from other places. The code is now in a module, covered by test, and centralized in a single place.

The next move should be to reuse the same code (methods and BLOCK from inc) from the remaining places (acq and serial, detail pages, etc.)

If we reach this point we will have all our add item forms and items tables using the exact same code, covered by tests. That will make any other improvements easier to implement.

Please see this as a continuous improvement of the current situation, not as the perfect solution we want to have at the end. I totally agree we should be using the REST API routes to achieve that goal, but we are not ready at all for that.
Building the table like it's done here is not achievable right now using the REST API, and was not the point anyway.

We need the item batch mod tool back running in background mode for 21.11.
Comment 21 Kyle M Hall (khall) 2021-09-10 12:47:26 UTC
(In reply to Jonathan Druart from comment #20)

I understand your intentions. I'm sure we have as many possible solutions as we do developers. If Koha and its community are to continue to be successful we need to adapt as technology and trends change. It's important that we keep that in mind is all.

I have reservations but this is definitely an improvement nevertheless.
Comment 22 Jonathan Druart 2021-10-06 13:33:59 UTC
Created attachment 125801 [details] [review]
Bug 28445: Isolate add item form to an include file

To ease reusability
Comment 23 Jonathan Druart 2021-10-06 13:34:04 UTC
Created attachment 125802 [details] [review]
Bug 28445: Move controller code to Koha::UI::Form::Builder::Item

To ease reusability
Comment 24 Jonathan Druart 2021-10-06 13:34:08 UTC
Created attachment 125803 [details] [review]
Bug 28445: Use Koha::UI::Form::Builder::Item from batchmod as well

Now that it's reusable, let use it somewhere else!
Comment 25 Jonathan Druart 2021-10-06 13:34:13 UTC
Created attachment 125804 [details] [review]
Bug 28445: Adjust code to handle regexs
Comment 26 Jonathan Druart 2021-10-06 13:34:19 UTC
Created attachment 125805 [details] [review]
Bug 28445: Start form with empty library for batch mod
Comment 27 Jonathan Druart 2021-10-06 13:34:24 UTC
Created attachment 125808 [details] [review]
Bug 28445: Use the task queue for the batch delete and update items tool

Here we go!

Disclaimer: this patch is huge and does many things, but splitting it in
several chunks would be time consuming and painful to rebase. However it
adds many tests and isolate/refactor code to make it way more reusable.

This patchset will make the "batch item modification" and "batch item
deletion" features use the task queue (reminder: Since bug 28158, and so
21.05.00, we do no longer use the old "background job" functionality and
the user does not get any info about the progress of the job).

More than that, more of the code to build an item form and a list of
items is now isolated in module (.pm) and include files (.inc)

We are reusing the changes made by bug 27526 that simplifies the way we
edit/create items (no more unecessary serialization Koha > MARC > MARCXML
> XML > HTML)

New module:
* Koha::BackgroundJob::BatchDeleteItem
    Subclass for process item deletion in batch
* Koha::BackgroundJob::BatchUpdateItem
    Subclass for process item modification in batch
* Koha::Item::Attributes
    We needed an object to represent item's attributes that are not
    mapped with a koha field (aka "more subfields xml")
    This module will help us to create the marcxml from a hashref and the
    reverse.
* Koha::UI::Form::Builder::Item
    The code that was used to build the add/edit item form is
    centralised in this module. In conjunction with the
    subfields_for_item BLOCK (from html_helpers.inc) it will be really
    easy to reuse this code in other places where the item form is used
    (acquisition and serials modules)
* Koha::UI::Table::Builder::Items
    Same as previously for the table. We are now using this table from 3
    different places (batch item mod, batch item del, backgroung job
    detail view) and the code is only in one place.
    To use with items_table_batchmod BLOCK (still from html_helpers.inc)

This patch is fixing some bugs about repeatable subfields and regex. A UI
change will reflect the limitation: if you want to apply a regex on a
subfield you cannot add several subfields for the same subfield code.

Test plan:

Prepare the ground:
- Make sure you are always using a bibliographic/item record using the framework
you are modifying!
- Add some subfields for items that are not mapped with a koha field
(note that you can use 'é' for more fun, don't try more funny
characters)
- Make some subfields (mapped and not mapped with a kohafield)
repeatable
- Add default values to some of your subfields

There are 4 main screens to test:
1. Add/edit item form
The behaviour should be the same before and after this patch.
See test plan from bug 27526.
Those 2 prefs must be tested:
    * SubfieldsToAllowForRestrictedEditing
    * SubfieldsToUseWhenPrefill

2. Batch modification
a. Fill some values, play with repeatable and regex.
Note that the behaviour in master was buggy, only the first value was modified by the regex:
    * With subfield = "a | b"
    1 value added with "new"
    => "new | b"

    * With subfield = "a | b"
    2 new fields "new1","new2"
    => "new2 | b"

Important note: For repeatable subfields, a regex will apply on the subfields in
the "concatenated form". To apply the regex on all the different subfields of a given
subfield code you must use the "g" modifier.
This could be improved later, but keep in mind that it's not a regression or behaviour
change.

b. Play with the "Populate fields with default values from default framework" checkbox

c. Use this tool to modify items and play with the different sysprefs that
interfer with it:
    * NewItemsDefaultLocation
    * SubfieldsToAllowForRestrictedBatchmod
    * MaxItemsToDisplayForBatchMod
    * MaxItemsToProcessForBatchMod

3. Batch deletion
a. Batch delete some items
b. Check items out and try to delete them
c. Use the "Delete records if no items remain" checkbox to delete
bibliographic records without remaining items.
d. Play with the following sysprefs and confirm that it works as
expected:
    * MaxItemsToDisplayForBatchDel
e. Stress the tool: Go to the confirmation screen with items that can be
deleted, don't request the job to be processed right away, but check the
item out before.

4. Background job detail view
You must have seen it already if you are curious and tested the above.
When a new modification or deletion batch is requested, the confirmation
screen will tell you that the job has enqueued. A link to the progress
of the job can be followed.
On this screen you will be able to see the result of the job once it's
fully processed.

QA notes:
* There are some FIXME's that are not blocker in my opinion. Feel free to
discuss them if you have suggestions.
* Do we still need MaxItemsToProcessForBatchMod?
* Prior to this patchset we had a "Return to the cataloging module" link
if we went from the cataloguing module and that the biblio was deleted.
We cannot longer know if the biblio will be deleted but we could display
a "Go to the cataloging module" link on the "job has been enqueued"
screen regardless from where we were coming from.
Comment 28 Jonathan Druart 2021-10-06 13:34:29 UTC
Created attachment 125810 [details] [review]
Bug 28445: UI change for repeatable and regex

This patch applies the changes describe in the main commit message about
the "limitation" and "the behaviour in master was buggy".
Comment 29 Jonathan Druart 2021-10-06 13:34:34 UTC
Created attachment 125811 [details] [review]
Bug 28445: Fix encoding issue with the background job's report
Comment 30 Jonathan Druart 2021-10-06 13:34:38 UTC
Created attachment 125812 [details] [review]
Bug 28445: Don't display the items if not finished yet
Comment 31 Jonathan Druart 2021-10-06 13:47:02 UTC
Created attachment 125817 [details] [review]
Bug 28445: Improve robustness of tests

* Could fail if not using the original sample data: build the AV we need
instead of relying on what is in the DB
* Libraries are ordered by name by default but if others have been added
the test may fail
Comment 32 Jonathan Druart 2021-10-06 13:50:41 UTC
Patches rebased on top of bug 23678 and bug 29149, also fixed some minor bugs and fragile tests.
Comment 33 Jonathan Druart 2021-10-07 09:22:41 UTC
Created attachment 125865 [details] [review]
Bug 28445: Improve robustness of tests

* Libraries are ordered by name by default but if others have been added
the test may fail
Comment 34 Jonathan Druart 2021-10-07 09:23:37 UTC
(In reply to Jonathan Druart from comment #33)
> Created attachment 125865 [details] [review] [review]
> Bug 28445: Improve robustness of tests
> 
> * Libraries are ordered by name by default but if others have been added
> the test may fail

Part of this patch has been moved out as a follow-up of bug 27526.
Comment 35 Nick Clemens (kidclamp) 2021-10-08 17:48:28 UTC
Notes for testing:
Edit the Default framework in order to ensure the new fields show during batch modification

Results:
Overall, things worked well, sent to background_jobs after an update, can return to batch mod

On additem.pl attempting to edit a repeated field only adds the value from the first field to all the repeated attempts, this is true on master as well

Blockers:
- Checkbox to 'delete' a subfield does not work, no changes are made
- If number of items to edit is over MaxItemsToDisplayForBatchMod, but below  MaxItemsToProcessForBatchMod the form does not show and no edits can be made
- MaxItemsToDisplayForBatchMod is not respected on background_jobs.pl, all items are displayed
- Progress may not be updated - hard to tell, was 0 until page reload loaded the full item list
- Batch deletion failed: Cannot enqueue this job. (The error was: Can't use string ("on") as an ARRAY ref while "strict refs" in use at /kohadevbox/koha/Koha/BackgroundJob/BatchDeleteItem.pm line 214. , see the Koha log file for more information).
Comment 36 Jonathan Druart 2021-10-11 14:33:05 UTC
Created attachment 126046 [details] [review]
Bug 28445: Fix subfields to blank

The value of the checkbox was not correct
Comment 37 Jonathan Druart 2021-10-11 14:33:10 UTC
Created attachment 126047 [details] [review]
Bug 28445: Don't surround the whole batch with a transaction

It makes more sense to commit when an item has successfully been modified and
so move the transaction inside the loop.

It also fixes the progress of the whole job.
Comment 38 Jonathan Druart 2021-10-11 14:36:35 UTC
Thanks Nick!

(In reply to Nick Clemens from comment #35)
> On additem.pl attempting to edit a repeated field only adds the value from
> the first field to all the repeated attempts, this is true on master as well

I don't understand. Can you detail please?
I made 952$u repeatable and I can create several subfields, edit, save. And it is working as I expect.

> Blockers:
> - Checkbox to 'delete' a subfield does not work, no changes are made

Will be fixed!

> - If number of items to edit is over MaxItemsToDisplayForBatchMod, but below
> MaxItemsToProcessForBatchMod the form does not show and no edits can be made

Cannot recreate.
Set MaxItemsToDisplayForBatchMod=2, MaxItemsToProcessForBatchMod=4, edit 3 barcodes.

I see "Too many items (3): You are editing more than 2 items in a batch, items will not be shown." and there is the form.

> - MaxItemsToDisplayForBatchMod is not respected on background_jobs.pl, all
> items are displayed

On purpose, do you think we should restore it? In my understanding it was to avoid the table on the first screen.

> - Progress may not be updated - hard to tell, was 0 until page reload loaded
> the full item list

It actually hide something different, the transaction was not made at the correct level. I think we should commit when an item has been modified. The previous behaviour was failing the whole job if something went wrong for an item. It's fixed now.

> - Batch deletion failed: Cannot enqueue this job. (The error was: Can't use
> string ("on") as an ARRAY ref while "strict refs" in use at
> /kohadevbox/koha/Koha/BackgroundJob/BatchDeleteItem.pm line 214. , see the
> Koha log file for more information).

Hum, cannot recreate that, but line 214 is: 
  214     my $delete_biblios = @{ $args->{delete_biblios} || [] };

That does not seem related at all, can you retry please?

Note that I have tested on top of the bug 27526 follow-up, not pushed yet. So results may vary.
Comment 39 Jonathan Druart 2021-10-12 17:52:33 UTC
Patchrs applied for me.
Did you try the remote branch, Nick?
Otherwise I will resubmit tomorrow.
Comment 40 Jonathan Druart 2021-10-13 06:49:08 UTC
Created attachment 126136 [details] [review]
Bug 28445: Use the task queue for the batch delete and update items tool

Here we go!

Disclaimer: this patch is huge and does many things, but splitting it in
several chunks would be time consuming and painful to rebase. However it
adds many tests and isolate/refactor code to make it way more reusable.

This patchset will make the "batch item modification" and "batch item
deletion" features use the task queue (reminder: Since bug 28158, and so
21.05.00, we do no longer use the old "background job" functionality and
the user does not get any info about the progress of the job).

More than that, more of the code to build an item form and a list of
items is now isolated in module (.pm) and include files (.inc)

We are reusing the changes made by bug 27526 that simplifies the way we
edit/create items (no more unecessary serialization Koha > MARC > MARCXML
> XML > HTML)

New module:
* Koha::BackgroundJob::BatchDeleteItem
    Subclass for process item deletion in batch
* Koha::BackgroundJob::BatchUpdateItem
    Subclass for process item modification in batch
* Koha::Item::Attributes
    We needed an object to represent item's attributes that are not
    mapped with a koha field (aka "more subfields xml")
    This module will help us to create the marcxml from a hashref and the
    reverse.
* Koha::UI::Form::Builder::Item
    The code that was used to build the add/edit item form is
    centralised in this module. In conjunction with the
    subfields_for_item BLOCK (from html_helpers.inc) it will be really
    easy to reuse this code in other places where the item form is used
    (acquisition and serials modules)
* Koha::UI::Table::Builder::Items
    Same as previously for the table. We are now using this table from 3
    different places (batch item mod, batch item del, backgroung job
    detail view) and the code is only in one place.
    To use with items_table_batchmod BLOCK (still from html_helpers.inc)

This patch is fixing some bugs about repeatable subfields and regex. A UI
change will reflect the limitation: if you want to apply a regex on a
subfield you cannot add several subfields for the same subfield code.

Test plan:

Prepare the ground:
- Make sure you are always using a bibliographic/item record using the framework
you are modifying!
- Add some subfields for items that are not mapped with a koha field
(note that you can use 'é' for more fun, don't try more funny
characters)
- Make some subfields (mapped and not mapped with a kohafield)
repeatable
- Add default values to some of your subfields

There are 4 main screens to test:
1. Add/edit item form
The behaviour should be the same before and after this patch.
See test plan from bug 27526.
Those 2 prefs must be tested:
    * SubfieldsToAllowForRestrictedEditing
    * SubfieldsToUseWhenPrefill

2. Batch modification
a. Fill some values, play with repeatable and regex.
Note that the behaviour in master was buggy, only the first value was modified by the regex:
    * With subfield = "a | b"
    1 value added with "new"
    => "new | b"

    * With subfield = "a | b"
    2 new fields "new1","new2"
    => "new2 | b"

Important note: For repeatable subfields, a regex will apply on the subfields in
the "concatenated form". To apply the regex on all the different subfields of a given
subfield code you must use the "g" modifier.
This could be improved later, but keep in mind that it's not a regression or behaviour
change.

b. Play with the "Populate fields with default values from default framework" checkbox

c. Use this tool to modify items and play with the different sysprefs that
interfer with it:
    * NewItemsDefaultLocation
    * SubfieldsToAllowForRestrictedBatchmod
    * MaxItemsToDisplayForBatchMod
    * MaxItemsToProcessForBatchMod

3. Batch deletion
a. Batch delete some items
b. Check items out and try to delete them
c. Use the "Delete records if no items remain" checkbox to delete
bibliographic records without remaining items.
d. Play with the following sysprefs and confirm that it works as
expected:
    * MaxItemsToDisplayForBatchDel
e. Stress the tool: Go to the confirmation screen with items that can be
deleted, don't request the job to be processed right away, but check the
item out before.

4. Background job detail view
You must have seen it already if you are curious and tested the above.
When a new modification or deletion batch is requested, the confirmation
screen will tell you that the job has enqueued. A link to the progress
of the job can be followed.
On this screen you will be able to see the result of the job once it's
fully processed.

QA notes:
* There are some FIXME's that are not blocker in my opinion. Feel free to
discuss them if you have suggestions.
* Do we still need MaxItemsToProcessForBatchMod?
* Prior to this patchset we had a "Return to the cataloging module" link
if we went from the cataloguing module and that the biblio was deleted.
We cannot longer know if the biblio will be deleted but we could display
a "Go to the cataloging module" link on the "job has been enqueued"
screen regardless from where we were coming from.
Comment 41 Jonathan Druart 2021-10-13 06:49:12 UTC
Created attachment 126137 [details] [review]
Bug 28445: UI change for repeatable and regex

This patch applies the changes describe in the main commit message about
the "limitation" and "the behaviour in master was buggy".
Comment 42 Jonathan Druart 2021-10-13 06:49:17 UTC
Created attachment 126138 [details] [review]
Bug 28445: Fix encoding issue with the background job's report
Comment 43 Jonathan Druart 2021-10-13 06:49:22 UTC
Created attachment 126139 [details] [review]
Bug 28445: Don't display the items if not finished yet
Comment 44 Jonathan Druart 2021-10-13 06:49:27 UTC
Created attachment 126140 [details] [review]
Bug 28445: Improve robustness of tests

* Libraries are ordered by name by default but if others have been added
the test may fail
Comment 45 Jonathan Druart 2021-10-13 06:49:33 UTC
Created attachment 126141 [details] [review]
Bug 28445: Fix subfields to blank

The value of the checkbox was not correct
Comment 46 Jonathan Druart 2021-10-13 06:49:40 UTC
Created attachment 126142 [details] [review]
Bug 28445: Don't surround the whole batch with a transaction

It makes more sense to commit when an item has successfully been modified and
so move the transaction inside the loop.

It also fixes the progress of the whole job.
Comment 47 Nick Clemens (kidclamp) 2021-10-13 14:02:45 UTC
(In reply to Jonathan Druart from comment #38)
> (In reply to Nick Clemens from comment #35)
> > On additem.pl attempting to edit a repeated field only adds the value from
> > the first field to all the repeated attempts, this is true on master as well
> 
> I don't understand. Can you detail please?
> I made 952$u repeatable and I can create several subfields, edit, save. And
> it is working as I expect.

I added new subfield "A" - the values repeat
On master I also made "o" repeatable and had same issue
With these patches "o" worked fine, "A" always repeated first value and during batch mod when I used the "A" field with two values a single record was often skipped (18 of 19 items updated), or all 19 would be updated, but 18 would repeat first value and the 19th would get the second
Something is weird there 

> > - MaxItemsToDisplayForBatchMod is not respected on background_jobs.pl, all
> > items are displayed
> 
> On purpose, do you think we should restore it? In my understanding it was to
> avoid the table on the first screen.

On master the items are not shown in results, I think we should do the same here - don't show more items than allowed in a batch modify job result

> > - Batch deletion failed: Cannot enqueue this job. (The error was: Can't use
> > string ("on") as an ARRAY ref while "strict refs" in use at
> > /kohadevbox/koha/Koha/BackgroundJob/BatchDeleteItem.pm line 214. , see the
> > Koha log file for more information).
> 
> Hum, cannot recreate that, but line 214 is: 
>   214     my $delete_biblios = @{ $args->{delete_biblios} || [] };
> 
> That does not seem related at all, can you retry please?

I get the error when I check "Delete records if no items remain."
Comment 48 Jonathan Druart 2021-10-13 15:18:25 UTC
(In reply to Nick Clemens from comment #47)
> (In reply to Jonathan Druart from comment #38)
> > (In reply to Nick Clemens from comment #35)
> > > On additem.pl attempting to edit a repeated field only adds the value from
> > > the first field to all the repeated attempts, this is true on master as well
> > 
> > I don't understand. Can you detail please?
> > I made 952$u repeatable and I can create several subfields, edit, save. And
> > it is working as I expect.
> 
> I added new subfield "A" - the values repeat
> On master I also made "o" repeatable and had same issue
> With these patches "o" worked fine, "A" always repeated first value and
> during batch mod when I used the "A" field with two values a single record
> was often skipped (18 of 19 items updated), or all 19 would be updated, but
> 18 would repeat first value and the 19th would get the second
> Something is weird there 

I do recreate, but it's the expected behaviour I think.
If you have "A" defined for the default framework, but not for BK, then the items using BK won't have the values added to "A".

> > > - MaxItemsToDisplayForBatchMod is not respected on background_jobs.pl, all
> > > items are displayed
> > 
> > On purpose, do you think we should restore it? In my understanding it was to
> > avoid the table on the first screen.
> 
> On master the items are not shown in results, I think we should do the same
> here - don't show more items than allowed in a batch modify job result

Ok, will follow-up.

> > > - Batch deletion failed: Cannot enqueue this job. (The error was: Can't use
> > > string ("on") as an ARRAY ref while "strict refs" in use at
> > > /kohadevbox/koha/Koha/BackgroundJob/BatchDeleteItem.pm line 214. , see the
> > > Koha log file for more information).
> > 
> > Hum, cannot recreate that, but line 214 is: 
> >   214     my $delete_biblios = @{ $args->{delete_biblios} || [] };
> > 
> > That does not seem related at all, can you retry please?
> 
> I get the error when I check "Delete records if no items remain."

Oops, silly mistake. Will fix.
Comment 49 Jonathan Druart 2021-10-13 15:19:52 UTC
Created attachment 126217 [details] [review]
Bug 28445: Don't surround the whole batch with a transaction

It makes more sense to commit when an item has successfully been modified and
so move the transaction inside the loop.

It also fixes the progress of the whole job.
Comment 50 Jonathan Druart 2021-10-13 15:19:57 UTC
Created attachment 126218 [details] [review]
Bug 28445: delete_biblios is a boolean

Will fix:
Batch deletion failed: Cannot enqueue this job. (The error was: Can't use string ("on") as an ARRAY ref while "strict refs" in use at /kohadevbox/koha/Koha/BackgroundJob/BatchDeleteItem.pm line 214. , see the Koha log file for more information).
Comment 51 Jonathan Druart 2021-10-13 15:27:51 UTC
Created attachment 126219 [details] [review]
Bug 28445: Respect MaxItemsToDisplayForBatchMod from job detail view
Comment 52 Jonathan Druart 2021-10-13 15:55:43 UTC
Created attachment 126223 [details] [review]
Bug 28445: Split and join on ' | ' instead of '|' for repeatable

Is that correct?!
Comment 53 Jonathan Druart 2021-10-13 15:56:58 UTC
(In reply to Jonathan Druart from comment #48)
> (In reply to Nick Clemens from comment #47)
> > (In reply to Jonathan Druart from comment #38)
> > > (In reply to Nick Clemens from comment #35)
> > > > On additem.pl attempting to edit a repeated field only adds the value from
> > > > the first field to all the repeated attempts, this is true on master as well
> > > 
> > > I don't understand. Can you detail please?
> > > I made 952$u repeatable and I can create several subfields, edit, save. And
> > > it is working as I expect.
> > 
> > I added new subfield "A" - the values repeat
> > On master I also made "o" repeatable and had same issue
> > With these patches "o" worked fine, "A" always repeated first value and
> > during batch mod when I used the "A" field with two values a single record
> > was often skipped (18 of 19 items updated), or all 19 would be updated, but
> > 18 would repeat first value and the 19th would get the second
> > Something is weird there 
> 
> I do recreate, but it's the expected behaviour I think.
> If you have "A" defined for the default framework, but not for BK, then the
> items using BK won't have the values added to "A".

The problem was when using the background job then the cataloguing edit form.
There was a mismatch with the separator for repeatable subfields.

It's fixed now, but I am wondering if split/join on ' | ' instead of '|' is correct.
Comment 54 Jonathan Druart 2021-10-13 16:02:18 UTC
(In reply to Jonathan Druart from comment #53)
> It's fixed now, but I am wondering if split/join on ' | ' instead of '|' is
> correct.

Confirmed on 21.05, we were using spaces prior to bug 27526 and this one.
Comment 55 Jonathan Druart 2021-10-15 14:02:14 UTC
Created attachment 126328 [details] [review]
Bug 28445: Adjust code to handle regexs
Comment 56 Jonathan Druart 2021-10-15 14:02:19 UTC
Created attachment 126329 [details] [review]
Bug 28445: Start form with empty library for batch mod
Comment 57 Jonathan Druart 2021-10-15 14:02:25 UTC
Created attachment 126330 [details] [review]
Bug 28445: Use the task queue for the batch delete and update items tool

Here we go!

Disclaimer: this patch is huge and does many things, but splitting it in
several chunks would be time consuming and painful to rebase. However it
adds many tests and isolate/refactor code to make it way more reusable.

This patchset will make the "batch item modification" and "batch item
deletion" features use the task queue (reminder: Since bug 28158, and so
21.05.00, we do no longer use the old "background job" functionality and
the user does not get any info about the progress of the job).

More than that, more of the code to build an item form and a list of
items is now isolated in module (.pm) and include files (.inc)

We are reusing the changes made by bug 27526 that simplifies the way we
edit/create items (no more unecessary serialization Koha > MARC > MARCXML
> XML > HTML)

New module:
* Koha::BackgroundJob::BatchDeleteItem
    Subclass for process item deletion in batch
* Koha::BackgroundJob::BatchUpdateItem
    Subclass for process item modification in batch
* Koha::Item::Attributes
    We needed an object to represent item's attributes that are not
    mapped with a koha field (aka "more subfields xml")
    This module will help us to create the marcxml from a hashref and the
    reverse.
* Koha::UI::Form::Builder::Item
    The code that was used to build the add/edit item form is
    centralised in this module. In conjunction with the
    subfields_for_item BLOCK (from html_helpers.inc) it will be really
    easy to reuse this code in other places where the item form is used
    (acquisition and serials modules)
* Koha::UI::Table::Builder::Items
    Same as previously for the table. We are now using this table from 3
    different places (batch item mod, batch item del, backgroung job
    detail view) and the code is only in one place.
    To use with items_table_batchmod BLOCK (still from html_helpers.inc)

This patch is fixing some bugs about repeatable subfields and regex. A UI
change will reflect the limitation: if you want to apply a regex on a
subfield you cannot add several subfields for the same subfield code.

Test plan:

Prepare the ground:
- Make sure you are always using a bibliographic/item record using the framework
you are modifying!
- Add some subfields for items that are not mapped with a koha field
(note that you can use 'é' for more fun, don't try more funny
characters)
- Make some subfields (mapped and not mapped with a kohafield)
repeatable
- Add default values to some of your subfields

There are 4 main screens to test:
1. Add/edit item form
The behaviour should be the same before and after this patch.
See test plan from bug 27526.
Those 2 prefs must be tested:
    * SubfieldsToAllowForRestrictedEditing
    * SubfieldsToUseWhenPrefill

2. Batch modification
a. Fill some values, play with repeatable and regex.
Note that the behaviour in master was buggy, only the first value was modified by the regex:
    * With subfield = "a | b"
    1 value added with "new"
    => "new | b"

    * With subfield = "a | b"
    2 new fields "new1","new2"
    => "new2 | b"

Important note: For repeatable subfields, a regex will apply on the subfields in
the "concatenated form". To apply the regex on all the different subfields of a given
subfield code you must use the "g" modifier.
This could be improved later, but keep in mind that it's not a regression or behaviour
change.

b. Play with the "Populate fields with default values from default framework" checkbox

c. Use this tool to modify items and play with the different sysprefs that
interfer with it:
    * NewItemsDefaultLocation
    * SubfieldsToAllowForRestrictedBatchmod
    * MaxItemsToDisplayForBatchMod
    * MaxItemsToProcessForBatchMod

3. Batch deletion
a. Batch delete some items
b. Check items out and try to delete them
c. Use the "Delete records if no items remain" checkbox to delete
bibliographic records without remaining items.
d. Play with the following sysprefs and confirm that it works as
expected:
    * MaxItemsToDisplayForBatchDel
e. Stress the tool: Go to the confirmation screen with items that can be
deleted, don't request the job to be processed right away, but check the
item out before.

4. Background job detail view
You must have seen it already if you are curious and tested the above.
When a new modification or deletion batch is requested, the confirmation
screen will tell you that the job has enqueued. A link to the progress
of the job can be followed.
On this screen you will be able to see the result of the job once it's
fully processed.

QA notes:
* There are some FIXME's that are not blocker in my opinion. Feel free to
discuss them if you have suggestions.
* Do we still need MaxItemsToProcessForBatchMod?
* Prior to this patchset we had a "Return to the cataloging module" link
if we went from the cataloguing module and that the biblio was deleted.
We cannot longer know if the biblio will be deleted but we could display
a "Go to the cataloging module" link on the "job has been enqueued"
screen regardless from where we were coming from.
Comment 58 Jonathan Druart 2021-10-15 14:02:30 UTC
Created attachment 126331 [details] [review]
Bug 28445: UI change for repeatable and regex

This patch applies the changes describe in the main commit message about
the "limitation" and "the behaviour in master was buggy".
Comment 59 Jonathan Druart 2021-10-15 14:02:36 UTC
Created attachment 126332 [details] [review]
Bug 28445: Fix encoding issue with the background job's report
Comment 60 Jonathan Druart 2021-10-15 14:02:40 UTC
Created attachment 126333 [details] [review]
Bug 28445: Don't display the items if not finished yet
Comment 61 Jonathan Druart 2021-10-15 14:02:45 UTC
Created attachment 126334 [details] [review]
Bug 28445: Improve robustness of tests

* Libraries are ordered by name by default but if others have been added
the test may fail
Comment 62 Jonathan Druart 2021-10-15 14:02:50 UTC
Created attachment 126335 [details] [review]
Bug 28445: Fix subfields to blank

The value of the checkbox was not correct
Comment 63 Jonathan Druart 2021-10-15 14:02:55 UTC
Created attachment 126336 [details] [review]
Bug 28445: Don't surround the whole batch with a transaction

It makes more sense to commit when an item has successfully been modified and
so move the transaction inside the loop.

It also fixes the progress of the whole job.
Comment 64 Jonathan Druart 2021-10-15 14:03:00 UTC
Created attachment 126337 [details] [review]
Bug 28445: delete_biblios is a boolean

Will fix:
Batch deletion failed: Cannot enqueue this job. (The error was: Can't use string ("on") as an ARRAY ref while "strict refs" in use at /kohadevbox/koha/Koha/BackgroundJob/BatchDeleteItem.pm line 214. , see the Koha log file for more information).
Comment 65 Jonathan Druart 2021-10-15 14:03:06 UTC
Created attachment 126338 [details] [review]
Bug 28445: Respect MaxItemsToDisplayForBatchMod from job detail view
Comment 66 Jonathan Druart 2021-10-15 14:03:11 UTC
Created attachment 126339 [details] [review]
Bug 28445: Split and join on ' | ' instead of '|' for repeatable

Is that correct?!
Comment 67 Nick Clemens (kidclamp) 2021-10-15 16:41:38 UTC
Created attachment 126352 [details] [review]
Bug 28445: Isolate add item form to an include file

To ease reusability

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 68 Nick Clemens (kidclamp) 2021-10-15 16:41:43 UTC
Created attachment 126353 [details] [review]
Bug 28445: Move controller code to Koha::UI::Form::Builder::Item

To ease reusability

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 69 Nick Clemens (kidclamp) 2021-10-15 16:41:46 UTC
Created attachment 126354 [details] [review]
Bug 28445: Use Koha::UI::Form::Builder::Item from batchmod as well

Now that it's reusable, let use it somewhere else!

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 70 Nick Clemens (kidclamp) 2021-10-15 16:41:50 UTC
Created attachment 126355 [details] [review]
Bug 28445: Adjust code to handle regexs

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 71 Nick Clemens (kidclamp) 2021-10-15 16:41:54 UTC
Created attachment 126356 [details] [review]
Bug 28445: Start form with empty library for batch mod

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 72 Nick Clemens (kidclamp) 2021-10-15 16:41:58 UTC
Created attachment 126357 [details] [review]
Bug 28445: Use the task queue for the batch delete and update items tool

Here we go!

Disclaimer: this patch is huge and does many things, but splitting it in
several chunks would be time consuming and painful to rebase. However it
adds many tests and isolate/refactor code to make it way more reusable.

This patchset will make the "batch item modification" and "batch item
deletion" features use the task queue (reminder: Since bug 28158, and so
21.05.00, we do no longer use the old "background job" functionality and
the user does not get any info about the progress of the job).

More than that, more of the code to build an item form and a list of
items is now isolated in module (.pm) and include files (.inc)

We are reusing the changes made by bug 27526 that simplifies the way we
edit/create items (no more unecessary serialization Koha > MARC > MARCXML
> XML > HTML)

New module:
* Koha::BackgroundJob::BatchDeleteItem
    Subclass for process item deletion in batch
* Koha::BackgroundJob::BatchUpdateItem
    Subclass for process item modification in batch
* Koha::Item::Attributes
    We needed an object to represent item's attributes that are not
    mapped with a koha field (aka "more subfields xml")
    This module will help us to create the marcxml from a hashref and the
    reverse.
* Koha::UI::Form::Builder::Item
    The code that was used to build the add/edit item form is
    centralised in this module. In conjunction with the
    subfields_for_item BLOCK (from html_helpers.inc) it will be really
    easy to reuse this code in other places where the item form is used
    (acquisition and serials modules)
* Koha::UI::Table::Builder::Items
    Same as previously for the table. We are now using this table from 3
    different places (batch item mod, batch item del, backgroung job
    detail view) and the code is only in one place.
    To use with items_table_batchmod BLOCK (still from html_helpers.inc)

This patch is fixing some bugs about repeatable subfields and regex. A UI
change will reflect the limitation: if you want to apply a regex on a
subfield you cannot add several subfields for the same subfield code.

Test plan:

Prepare the ground:
- Make sure you are always using a bibliographic/item record using the framework
you are modifying!
- Add some subfields for items that are not mapped with a koha field
(note that you can use 'é' for more fun, don't try more funny
characters)
- Make some subfields (mapped and not mapped with a kohafield)
repeatable
- Add default values to some of your subfields

There are 4 main screens to test:
1. Add/edit item form
The behaviour should be the same before and after this patch.
See test plan from bug 27526.
Those 2 prefs must be tested:
    * SubfieldsToAllowForRestrictedEditing
    * SubfieldsToUseWhenPrefill

2. Batch modification
a. Fill some values, play with repeatable and regex.
Note that the behaviour in master was buggy, only the first value was modified by the regex:
    * With subfield = "a | b"
    1 value added with "new"
    => "new | b"

    * With subfield = "a | b"
    2 new fields "new1","new2"
    => "new2 | b"

Important note: For repeatable subfields, a regex will apply on the subfields in
the "concatenated form". To apply the regex on all the different subfields of a given
subfield code you must use the "g" modifier.
This could be improved later, but keep in mind that it's not a regression or behaviour
change.

b. Play with the "Populate fields with default values from default framework" checkbox

c. Use this tool to modify items and play with the different sysprefs that
interfer with it:
    * NewItemsDefaultLocation
    * SubfieldsToAllowForRestrictedBatchmod
    * MaxItemsToDisplayForBatchMod
    * MaxItemsToProcessForBatchMod

3. Batch deletion
a. Batch delete some items
b. Check items out and try to delete them
c. Use the "Delete records if no items remain" checkbox to delete
bibliographic records without remaining items.
d. Play with the following sysprefs and confirm that it works as
expected:
    * MaxItemsToDisplayForBatchDel
e. Stress the tool: Go to the confirmation screen with items that can be
deleted, don't request the job to be processed right away, but check the
item out before.

4. Background job detail view
You must have seen it already if you are curious and tested the above.
When a new modification or deletion batch is requested, the confirmation
screen will tell you that the job has enqueued. A link to the progress
of the job can be followed.
On this screen you will be able to see the result of the job once it's
fully processed.

QA notes:
* There are some FIXME's that are not blocker in my opinion. Feel free to
discuss them if you have suggestions.
* Do we still need MaxItemsToProcessForBatchMod?
* Prior to this patchset we had a "Return to the cataloging module" link
if we went from the cataloguing module and that the biblio was deleted.
We cannot longer know if the biblio will be deleted but we could display
a "Go to the cataloging module" link on the "job has been enqueued"
screen regardless from where we were coming from.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 73 Nick Clemens (kidclamp) 2021-10-15 16:42:02 UTC
Created attachment 126358 [details] [review]
Bug 28445: UI change for repeatable and regex

This patch applies the changes describe in the main commit message about
the "limitation" and "the behaviour in master was buggy".

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 74 Nick Clemens (kidclamp) 2021-10-15 16:42:06 UTC
Created attachment 126359 [details] [review]
Bug 28445: Fix encoding issue with the background job's report

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 75 Nick Clemens (kidclamp) 2021-10-15 16:42:09 UTC
Created attachment 126360 [details] [review]
Bug 28445: Don't display the items if not finished yet

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 76 Nick Clemens (kidclamp) 2021-10-15 16:42:13 UTC
Created attachment 126361 [details] [review]
Bug 28445: Improve robustness of tests

* Libraries are ordered by name by default but if others have been added
the test may fail

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 77 Nick Clemens (kidclamp) 2021-10-15 16:42:17 UTC
Created attachment 126362 [details] [review]
Bug 28445: Fix subfields to blank

The value of the checkbox was not correct

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 78 Nick Clemens (kidclamp) 2021-10-15 16:42:21 UTC
Created attachment 126363 [details] [review]
Bug 28445: Don't surround the whole batch with a transaction

It makes more sense to commit when an item has successfully been modified and
so move the transaction inside the loop.

It also fixes the progress of the whole job.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 79 Nick Clemens (kidclamp) 2021-10-15 16:42:25 UTC
Created attachment 126364 [details] [review]
Bug 28445: delete_biblios is a boolean

Will fix:
Batch deletion failed: Cannot enqueue this job. (The error was: Can't use string ("on") as an ARRAY ref while "strict refs" in use at /kohadevbox/koha/Koha/BackgroundJob/BatchDeleteItem.pm line 214. , see the Koha log file for more information).

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 80 Nick Clemens (kidclamp) 2021-10-15 16:42:29 UTC
Created attachment 126365 [details] [review]
Bug 28445: Respect MaxItemsToDisplayForBatchMod from job detail view

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 81 Nick Clemens (kidclamp) 2021-10-15 16:42:33 UTC
Created attachment 126366 [details] [review]
Bug 28445: Split and join on ' | ' instead of '|' for repeatable

Is that correct?!

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 82 Nick Clemens (kidclamp) 2021-10-15 16:42:37 UTC
Created attachment 126367 [details] [review]
Bug 28445: (follow-up) Display count of modified items when too many to display
Comment 83 Tomás Cohen Arazi (tcohen) 2021-10-15 18:50:05 UTC
Created attachment 126374 [details] [review]
Bug 28445: Isolate add item form to an include file

To ease reusability

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 84 Tomás Cohen Arazi (tcohen) 2021-10-15 18:50:10 UTC
Created attachment 126375 [details] [review]
Bug 28445: Move controller code to Koha::UI::Form::Builder::Item

To ease reusability

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 85 Tomás Cohen Arazi (tcohen) 2021-10-15 18:50:15 UTC
Created attachment 126376 [details] [review]
Bug 28445: Use Koha::UI::Form::Builder::Item from batchmod as well

Now that it's reusable, let use it somewhere else!

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 86 Tomás Cohen Arazi (tcohen) 2021-10-15 18:50:20 UTC
Created attachment 126377 [details] [review]
Bug 28445: Adjust code to handle regexs

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 87 Tomás Cohen Arazi (tcohen) 2021-10-15 18:50:25 UTC
Created attachment 126378 [details] [review]
Bug 28445: Start form with empty library for batch mod

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 88 Tomás Cohen Arazi (tcohen) 2021-10-15 18:50:31 UTC
Created attachment 126379 [details] [review]
Bug 28445: Use the task queue for the batch delete and update items tool

Here we go!

Disclaimer: this patch is huge and does many things, but splitting it in
several chunks would be time consuming and painful to rebase. However it
adds many tests and isolate/refactor code to make it way more reusable.

This patchset will make the "batch item modification" and "batch item
deletion" features use the task queue (reminder: Since bug 28158, and so
21.05.00, we do no longer use the old "background job" functionality and
the user does not get any info about the progress of the job).

More than that, more of the code to build an item form and a list of
items is now isolated in module (.pm) and include files (.inc)

We are reusing the changes made by bug 27526 that simplifies the way we
edit/create items (no more unecessary serialization Koha > MARC > MARCXML
> XML > HTML)

New module:
* Koha::BackgroundJob::BatchDeleteItem
    Subclass for process item deletion in batch
* Koha::BackgroundJob::BatchUpdateItem
    Subclass for process item modification in batch
* Koha::Item::Attributes
    We needed an object to represent item's attributes that are not
    mapped with a koha field (aka "more subfields xml")
    This module will help us to create the marcxml from a hashref and the
    reverse.
* Koha::UI::Form::Builder::Item
    The code that was used to build the add/edit item form is
    centralised in this module. In conjunction with the
    subfields_for_item BLOCK (from html_helpers.inc) it will be really
    easy to reuse this code in other places where the item form is used
    (acquisition and serials modules)
* Koha::UI::Table::Builder::Items
    Same as previously for the table. We are now using this table from 3
    different places (batch item mod, batch item del, backgroung job
    detail view) and the code is only in one place.
    To use with items_table_batchmod BLOCK (still from html_helpers.inc)

This patch is fixing some bugs about repeatable subfields and regex. A UI
change will reflect the limitation: if you want to apply a regex on a
subfield you cannot add several subfields for the same subfield code.

Test plan:

Prepare the ground:
- Make sure you are always using a bibliographic/item record using the framework
you are modifying!
- Add some subfields for items that are not mapped with a koha field
(note that you can use 'é' for more fun, don't try more funny
characters)
- Make some subfields (mapped and not mapped with a kohafield)
repeatable
- Add default values to some of your subfields

There are 4 main screens to test:
1. Add/edit item form
The behaviour should be the same before and after this patch.
See test plan from bug 27526.
Those 2 prefs must be tested:
    * SubfieldsToAllowForRestrictedEditing
    * SubfieldsToUseWhenPrefill

2. Batch modification
a. Fill some values, play with repeatable and regex.
Note that the behaviour in master was buggy, only the first value was modified by the regex:
    * With subfield = "a | b"
    1 value added with "new"
    => "new | b"

    * With subfield = "a | b"
    2 new fields "new1","new2"
    => "new2 | b"

Important note: For repeatable subfields, a regex will apply on the subfields in
the "concatenated form". To apply the regex on all the different subfields of a given
subfield code you must use the "g" modifier.
This could be improved later, but keep in mind that it's not a regression or behaviour
change.

b. Play with the "Populate fields with default values from default framework" checkbox

c. Use this tool to modify items and play with the different sysprefs that
interfer with it:
    * NewItemsDefaultLocation
    * SubfieldsToAllowForRestrictedBatchmod
    * MaxItemsToDisplayForBatchMod
    * MaxItemsToProcessForBatchMod

3. Batch deletion
a. Batch delete some items
b. Check items out and try to delete them
c. Use the "Delete records if no items remain" checkbox to delete
bibliographic records without remaining items.
d. Play with the following sysprefs and confirm that it works as
expected:
    * MaxItemsToDisplayForBatchDel
e. Stress the tool: Go to the confirmation screen with items that can be
deleted, don't request the job to be processed right away, but check the
item out before.

4. Background job detail view
You must have seen it already if you are curious and tested the above.
When a new modification or deletion batch is requested, the confirmation
screen will tell you that the job has enqueued. A link to the progress
of the job can be followed.
On this screen you will be able to see the result of the job once it's
fully processed.

QA notes:
* There are some FIXME's that are not blocker in my opinion. Feel free to
discuss them if you have suggestions.
* Do we still need MaxItemsToProcessForBatchMod?
* Prior to this patchset we had a "Return to the cataloging module" link
if we went from the cataloguing module and that the biblio was deleted.
We cannot longer know if the biblio will be deleted but we could display
a "Go to the cataloging module" link on the "job has been enqueued"
screen regardless from where we were coming from.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 89 Tomás Cohen Arazi (tcohen) 2021-10-15 18:50:36 UTC
Created attachment 126380 [details] [review]
Bug 28445: UI change for repeatable and regex

This patch applies the changes describe in the main commit message about
the "limitation" and "the behaviour in master was buggy".

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 90 Tomás Cohen Arazi (tcohen) 2021-10-15 18:50:41 UTC
Created attachment 126381 [details] [review]
Bug 28445: Fix encoding issue with the background job's report

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 91 Tomás Cohen Arazi (tcohen) 2021-10-15 18:50:46 UTC
Created attachment 126382 [details] [review]
Bug 28445: Don't display the items if not finished yet

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 92 Tomás Cohen Arazi (tcohen) 2021-10-15 18:50:51 UTC
Created attachment 126383 [details] [review]
Bug 28445: Improve robustness of tests

* Libraries are ordered by name by default but if others have been added
the test may fail

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 93 Tomás Cohen Arazi (tcohen) 2021-10-15 18:50:56 UTC
Created attachment 126384 [details] [review]
Bug 28445: Fix subfields to blank

The value of the checkbox was not correct

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 94 Tomás Cohen Arazi (tcohen) 2021-10-15 18:51:01 UTC
Created attachment 126385 [details] [review]
Bug 28445: Don't surround the whole batch with a transaction

It makes more sense to commit when an item has successfully been modified and
so move the transaction inside the loop.

It also fixes the progress of the whole job.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 95 Tomás Cohen Arazi (tcohen) 2021-10-15 18:51:06 UTC
Created attachment 126386 [details] [review]
Bug 28445: delete_biblios is a boolean

Will fix:
Batch deletion failed: Cannot enqueue this job. (The error was: Can't use string ("on") as an ARRAY ref while "strict refs" in use at /kohadevbox/koha/Koha/BackgroundJob/BatchDeleteItem.pm line 214. , see the Koha log file for more information).

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 96 Tomás Cohen Arazi (tcohen) 2021-10-15 18:51:12 UTC
Created attachment 126387 [details] [review]
Bug 28445: Respect MaxItemsToDisplayForBatchMod from job detail view

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 97 Tomás Cohen Arazi (tcohen) 2021-10-15 18:51:17 UTC
Created attachment 126388 [details] [review]
Bug 28445: Split and join on ' | ' instead of '|' for repeatable

Is that correct?!

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 98 Tomás Cohen Arazi (tcohen) 2021-10-15 18:51:22 UTC
Created attachment 126389 [details] [review]
Bug 28445: (follow-up) Display count of modified items when too many to display

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 99 Jonathan Druart 2021-10-18 09:33:59 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 100 Jonathan Druart 2021-10-28 08:53:14 UTC
Created attachment 127033 [details] [review]
Bug 28445: Add tests for subfields_to_prefill

I first wrote test for another thing but the code was actually correct.
So submitting the tests.

Note that the marc for more_subfields does not need to be passed, we can
build it when needed.
Comment 101 Jonathan Druart 2021-10-28 09:11:33 UTC
Last patch pushed to master.
Comment 102 Jonathan Druart 2021-10-28 10:06:03 UTC
Created attachment 127037 [details] [review]
Bug 28445: Correctly count modified items
Comment 103 Jonathan Druart 2021-10-28 10:06:09 UTC
Created attachment 127038 [details] [review]
Bug 28445: Remove modified_itemitemnumbers

Wrong substitution
Comment 104 Jonathan Druart 2021-10-28 10:06:15 UTC
Created attachment 127039 [details] [review]
Bug 28445: Fix LostItem call and display errors

Never a try without a catch, ever!
Comment 105 Jonathan Druart 2021-10-28 10:30:35 UTC
Last three patches pushed to master.
Comment 106 Jonathan Druart 2021-11-25 12:55:20 UTC
Created attachment 128007 [details] [review]
Bug 28445: Add diag for failing test

Temporary patch to help trying to fix the following random error:
     #   Failed test 'Item types should be sorted by description and an empty entries should be shown'
     #   at t/db_dependent/Koha/UI/Form/Builder/Item.t line 121.
     #     Structures begin differing at:
     #          $got->[12] = 'Be9T9FH5aO'
     #     $expected->[12] = 'REF'
     # Looks like you failed 1 test of 2.

 #   Failed test 'itemtypes'
 #   at t/db_dependent/Koha/UI/Form/Builder/Item.t line 130.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 107 Jonathan Druart 2021-11-25 12:55:39 UTC
Created attachment 128008 [details] [review]
Bug 28445: (follow-up) Add diag for failing test

Last diag was not enough (Koha_Master_D11/457)

         #   Failed test 'Item types should be sorted by description and an empty entries should be shown'
         #   at t/db_dependent/Koha/UI/Form/Builder/Item.t line 127.
         #     Structures begin differing at:
         #          $got->[11] = 'X_vvQE5'
         #     $expected->[11] = 'knHm32'
         # Itemtypes details: $VAR1 = [
         #           '',
         #           'EMVzKcGR',
         #           'x4S8_BaFCp',
         #           'Cp2BGkY',
         #           'BK',
         #           'CF',
         #           'CR',
         #           'bBvgmVAIzD',
         #           'WKyaQ_l',
         #           'Cjc0SKQz',
         #           'liic63',
         #           'X_vvQE5',
         #           'knHm32',
         #           'MP',
         #           'MX',
         #           'MU',
         #           'g_F5eNqA',
         #           'REF',
         #           'pkxaMSv',
         #           'VM'
         #         ];
         # $VAR2 = [
         #           'bBvgmVAIzD',
         #           'BK',
         #           'CF',
         #           'Cjc0SKQz',
         #           'Cp2BGkY',
         #           'CR',
         #           'EMVzKcGR',
         #           'g_F5eNqA',
         #           'knHm32',
         #           'liic63',
         #           'MP',
         #           'MU',
         #           'MX',
         #           'pkxaMSv',
         #           'REF',
         #           'VM',
         #           'WKyaQ_l',
         #           'X_vvQE5',
         #           'x4S8_BaFCp'
         #         ];
We needed the translated_description. It should be the same actually,
but maybe something weird is happening.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 108 Jonathan Druart 2021-11-25 12:56:13 UTC
Created attachment 128009 [details] [review]
Bug 28445: Prevent UI/Form/Builder/Item.t to fail randomly

MySQL and Perl don't order strings with _ identically which cause a
mismatch when comparing the itemtypes.
Comment 109 Jonathan Druart 2021-11-25 13:58:05 UTC
Last 3 patches pushed to master (the first one has been pushed earlier actually).