Bug 27526 - Remove Mod/AddItemFromMarc from additem.pl
Summary: Remove Mod/AddItemFromMarc from additem.pl
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords: rel_21_11_candidate, RM_priority
Depends on: 8976 27545
Blocks: 28445 28687 28694 28769
  Show dependency treegraph
 
Reported: 2021-01-22 12:19 UTC by Jonathan Druart
Modified: 2021-08-03 12:30 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 27526: Remove ModItemFromMarc from additem (12.58 KB, patch)
2021-01-22 12:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27526: Remove AddItemFromMarc from additem (15.96 KB, patch)
2021-01-22 12:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27526: Adjust code to use Koha::Items (48.29 KB, patch)
2021-01-29 06:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27526: Remove ModItemFromMarc from additem (12.58 KB, patch)
2021-03-05 11:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27526: Remove AddItemFromMarc from additem (15.73 KB, patch)
2021-03-05 11:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27526: Adjust code to use Koha::Items (48.29 KB, patch)
2021-03-05 11:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27526: Remove ModItemFromMarc from additem (12.55 KB, patch)
2021-06-03 12:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27526: Remove AddItemFromMarc from additem (15.73 KB, patch)
2021-06-03 12:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27526: Adjust code to use Koha::Items (47.36 KB, patch)
2021-06-03 12:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27526: Adjust code to use Koha::Items (52.30 KB, patch)
2021-06-22 14:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27526: Improve grep for date fields (1.25 KB, patch)
2021-06-23 08:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27526: Remove ModItemFromMarc from additem (12.53 KB, patch)
2021-07-07 21:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27526: Remove AddItemFromMarc from additem (15.78 KB, patch)
2021-07-07 21:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27526: Adjust code to use Koha::Items (52.27 KB, patch)
2021-07-07 21:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27526: Improve grep for date fields (1.31 KB, patch)
2021-07-07 21:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27526: Fix repeatable more subfields (1.24 KB, patch)
2021-07-08 13:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27526: Fix mandatory and important checks (1.89 KB, patch)
2021-07-08 13:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27526: Fix repeatable more subfields (1.29 KB, patch)
2021-07-08 14:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27526: Fix mandatory and important checks (1.94 KB, patch)
2021-07-08 14:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27526: Fix PrefillItem (4.11 KB, patch)
2021-07-09 08:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27526: Fix Add & duplicate (1.61 KB, patch)
2021-07-09 08:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27526: Fix Add multiple copies (970 bytes, patch)
2021-07-09 08:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27526: Remove ModItemFromMarc from additem (12.54 KB, patch)
2021-07-19 14:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27526: Remove AddItemFromMarc from additem (15.88 KB, patch)
2021-07-19 14:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27526: Adjust code to use Koha::Items (52.41 KB, patch)
2021-07-19 14:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27526: Improve grep for date fields (1.31 KB, patch)
2021-07-19 14:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27526: Fix repeatable more subfields (1.30 KB, patch)
2021-07-19 14:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27526: Fix mandatory and important checks (1.95 KB, patch)
2021-07-19 14:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27526: Fix PrefillItem (4.11 KB, patch)
2021-07-19 14:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27526: Fix Add & duplicate (1.61 KB, patch)
2021-07-19 14:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27526: Fix Add multiple copies (970 bytes, patch)
2021-07-19 14:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27526: Fix incorrect condition (935 bytes, patch)
2021-07-20 09:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27526: Fix incorrect condition (2.08 KB, patch)
2021-07-20 10:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27526: Fix empty string vs undef (1.09 KB, patch)
2021-07-20 10:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27526: Remove uneeded call to TransformMarcToKoha (8.94 KB, patch)
2021-07-20 12:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27526: Remove uneeded call to TransformMarcToKoha (9.06 KB, patch)
2021-07-21 08:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27526: Fix encoding issue on subfield (1.70 KB, patch)
2021-07-21 09:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27526: Fix cn_source display (1.31 KB, patch)
2021-07-23 13:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27526: Add missing POD (1.18 KB, patch)
2021-07-30 14:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27526: Add tests for columns_to_str and host_items (6.67 KB, patch)
2021-08-02 12:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27526: Add tests for columns_to_str and host_items (6.75 KB, patch)
2021-08-03 12:30 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-01-22 12:19:38 UTC
There is a lot of code in additem.pl that is transforming an item representation from/to HTML, XML, MARC::Record, Koha::Item
We really don't need all this overhead, having the Koha::Item's attributes would be enough to update/create the item into the DB (it's no longer in the biblio MARC::Record!)

Of course other places where we add/update items are valid as well: acquisition, batch item modification, etc.
Comment 1 Jonathan Druart 2021-01-22 12:20:30 UTC
Created attachment 115679 [details] [review]
Bug 27526: Remove ModItemFromMarc from additem
Comment 2 Jonathan Druart 2021-01-22 12:20:33 UTC
Created attachment 115680 [details] [review]
Bug 27526: Remove AddItemFromMarc from additem
Comment 3 Jonathan Druart 2021-01-22 12:23:38 UTC
Those two patches are a first (POC) step. Lot of code from the controller and template must be adjusted and removed.

I would like to know how people are feeling about this move and if some of you can commit to test and review the patches if I am continuing the rewrite.
Comment 4 Nick Clemens 2021-01-22 12:26:35 UTC
I would happily test these, this code is confusing and due for an overhaul
Comment 5 Martin Renvoize 2021-01-22 12:38:39 UTC
Count me in for testing/review too.. it would be a lovely area to clean up.
Comment 6 Tomás Cohen Arazi 2021-01-22 12:56:52 UTC
I expected to see Koha::Item->new_from_marc({  })
Comment 7 Jonathan Druart 2021-01-22 14:00:17 UTC
(In reply to Tomás Cohen Arazi from comment #6)
> I expected to see Koha::Item->new_from_marc({  })

No, the idea is to remove MARC::Record completely when dealing with items.
Comment 8 Tomás Cohen Arazi 2021-01-22 14:02:19 UTC
(In reply to Jonathan Druart from comment #7)
> (In reply to Tomás Cohen Arazi from comment #6)
> > I expected to see Koha::Item->new_from_marc({  })
> 
> No, the idea is to remove MARC::Record completely when dealing with items.

Yes. When I read the patches, I noticed you were tackling the cataloging form. But there is also import.
Comment 9 David Cook 2021-01-24 22:55:13 UTC
(In reply to Jonathan Druart from comment #3)
> Those two patches are a first (POC) step. Lot of code from the controller
> and template must be adjusted and removed.
> 

Do you mean that you're planning to move the new replacement code from additem.pl and put it in a module?
Comment 10 Jonathan Druart 2021-01-29 06:27:32 UTC
Created attachment 116006 [details] [review]
Bug 27526: Adjust code to use Koha::Items

Not that we removed all the transformations of the item and are using
Koha::Item from DB to TT (and the other way around), some code needs
adjustments.

- Retrieve host items can be simplified (see Koha::Biblio->host_items)
- Some TT variables have been renamed for better understanding
- Koha::Item->columns_to_str return a hashref with the representation
string of the columns. A date will return the value how it must be
displayed, using output_pref. A subfield linked with a AV will be
replaced with the AV's description.
- LastCreatedItem cookie serializes and stores Koha::Item->unblessed,
no longer the MARC::Record

Change in behaviour: If a subfield is linked with a AV cat and the value
is not a valid AV, before this patch the column was displayed with an
empty value. Now the column is hidden, it's considered empty.
In the sample data it happens with itemlost (0) and withdrawn (0).

Test plan:
1. Test the Prefill
 a. Turn PrefillItem on
 b. Fill in SubfieldsToUseWhenPrefill with some subfield codes
 c. Catalogue an item, save
 => The values from subfields listed in SubfieldsToUseWhenPrefill must be
 kept
2. more subfields
 a. Add subfields that are not linked with a koha field (k is available)
 b. Create an item and fill in all the values
 c. Confirm that non linked subfields are stored and displayed correctly
 d. Try with a "more subfield" that is linked with an authorised value
 category
3. Test barcode values
4. Test the different "Add" buttons at the bottom of the form
Comment 11 Jonathan Druart 2021-01-29 06:32:53 UTC
This patch is not ready, at least tests are missing. But I really would like to get early feedbacks on it to know if I continue working on it.

To understand a bit more the method and how I wrote this patch, I think it may be easier to read the step by step approach I've pushed remotely: https://gitlab.com/joubu/Koha/-/commits/bug_27526_dirty

If we all agree it's the way to go, we could then extend the code (and so make it even more flexible) to other areas.
Comment 12 Marcel de Rooy 2021-01-29 08:39:29 UTC
+        if ( $c eq 'more_subfields_xml' ) {
+            my @more_subfields_xml = $input->multi_param("items.more_subfields_xml");
+            my @unlinked_item_subfields;
+            for my $subfield ( @more_subfields_xml ) {
+                my $v = $input->param('items.more_subfields_xml_' . $subfield);
+                push @unlinked_item_subfields, $subfield, $v;
+            }
+            if ( @unlinked_item_subfields ) {
+                my $marc = MARC::Record->new();
+                # use of tag 999 is arbitrary, and doesn't need to match the item tag
+                # used in the framework
+                $marc->append_fields(MARC::Field->new('999', ' ', ' ', @unlinked_item_subfields));
+                $marc->encoding("UTF-8");
+                $item->more_subfields_xml($marc->as_xml("USMARC"));
+                next;
+            }
+            $item->more_subfields_xml(undef);
+        } else {
+            my $v = $input->param("items.".$c);
+            next unless defined $v;
+            $item->$c($v);
+        }

Is this the time to get rid of the MARC stuff for items completely?
Do we really need more_subfields_xml or could we resolve otherwise?
Comment 13 Marcel de Rooy 2021-01-29 08:39:56 UTC
(In reply to Jonathan Druart from comment #7)
> (In reply to Tomás Cohen Arazi from comment #6)
> > I expected to see Koha::Item->new_from_marc({  })
> 
> No, the idea is to remove MARC::Record completely when dealing with items.

^^
Comment 14 Jonathan Druart 2021-01-29 08:48:58 UTC
Well, that's not what I meant. *almost* completely :D

But yes, we could move them to a new DB table.
Comment 15 Tomás Cohen Arazi 2021-02-04 13:17:38 UTC
(In reply to Marcel de Rooy from comment #12)
> +        if ( $c eq 'more_subfields_xml' ) {
> +            my @more_subfields_xml =
> $input->multi_param("items.more_subfields_xml");
> +            my @unlinked_item_subfields;
> +            for my $subfield ( @more_subfields_xml ) {
> +                my $v = $input->param('items.more_subfields_xml_' .
> $subfield);
> +                push @unlinked_item_subfields, $subfield, $v;
> +            }
> +            if ( @unlinked_item_subfields ) {
> +                my $marc = MARC::Record->new();
> +                # use of tag 999 is arbitrary, and doesn't need to match
> the item tag
> +                # used in the framework
> +                $marc->append_fields(MARC::Field->new('999', ' ', ' ',
> @unlinked_item_subfields));
> +                $marc->encoding("UTF-8");
> +                $item->more_subfields_xml($marc->as_xml("USMARC"));
> +                next;
> +            }
> +            $item->more_subfields_xml(undef);
> +        } else {
> +            my $v = $input->param("items.".$c);
> +            next unless defined $v;
> +            $item->$c($v);
> +        }
> 
> Is this the time to get rid of the MARC stuff for items completely?
> Do we really need more_subfields_xml or could we resolve otherwise?

+1, please file that bug!
Comment 16 Jonathan Druart 2021-03-03 11:25:55 UTC
Can I get some testing on this please?
Comment 17 Jonathan Druart 2021-03-05 11:03:39 UTC
Created attachment 117806 [details] [review]
Bug 27526: Remove ModItemFromMarc from additem
Comment 18 Jonathan Druart 2021-03-05 11:03:43 UTC
Created attachment 117807 [details] [review]
Bug 27526: Remove AddItemFromMarc from additem
Comment 19 Jonathan Druart 2021-03-05 11:03:47 UTC
Created attachment 117808 [details] [review]
Bug 27526: Adjust code to use Koha::Items

Not that we removed all the transformations of the item and are using
Koha::Item from DB to TT (and the other way around), some code needs
adjustments.

- Retrieve host items can be simplified (see Koha::Biblio->host_items)
- Some TT variables have been renamed for better understanding
- Koha::Item->columns_to_str return a hashref with the representation
string of the columns. A date will return the value how it must be
displayed, using output_pref. A subfield linked with a AV will be
replaced with the AV's description.
- LastCreatedItem cookie serializes and stores Koha::Item->unblessed,
no longer the MARC::Record

Change in behaviour: If a subfield is linked with a AV cat and the value
is not a valid AV, before this patch the column was displayed with an
empty value. Now the column is hidden, it's considered empty.
In the sample data it happens with itemlost (0) and withdrawn (0).

Test plan:
1. Test the Prefill
 a. Turn PrefillItem on
 b. Fill in SubfieldsToUseWhenPrefill with some subfield codes
 c. Catalogue an item, save
 => The values from subfields listed in SubfieldsToUseWhenPrefill must be
 kept
2. more subfields
 a. Add subfields that are not linked with a koha field (k is available)
 b. Create an item and fill in all the values
 c. Confirm that non linked subfields are stored and displayed correctly
 d. Try with a "more subfield" that is linked with an authorised value
 category
3. Test barcode values
4. Test the different "Add" buttons at the bottom of the form
Comment 20 Martin Renvoize 2021-04-01 12:40:12 UTC
I've somewhat lost track of this one I'm afraid.. I'll try to take a look next week.. do you think we're still aiming for 21.05 for it Jonathan?
Comment 21 Jonathan Druart 2021-04-01 16:46:56 UTC
(In reply to Martin Renvoize from comment #20)
> I've somewhat lost track of this one I'm afraid.. I'll try to take a look
> next week.. do you think we're still aiming for 21.05 for it Jonathan?

Yes, I am still willing to push it for 21.05.
Comment 22 Tomás Cohen Arazi 2021-04-01 16:48:18 UTC
(In reply to Jonathan Druart from comment #21)
> (In reply to Martin Renvoize from comment #20)
> > I've somewhat lost track of this one I'm afraid.. I'll try to take a look
> > next week.. do you think we're still aiming for 21.05 for it Jonathan?
> 
> Yes, I am still willing to push it for 21.05.

I'm playing with this at the moment.
Comment 23 Victor Grousset/tuxayo 2021-05-01 00:47:01 UTC
>  c. Confirm that non linked subfields are stored and displayed correctly

- item edition list
- MARC details
- OPAC marc view

Is that it?

> d. Try with a "more subfield" that is linked with an authorised value
 category

what is a "more subfield"?

> 3. Test barcode values

just put a barcode and using it afterwards?
Comment 24 Jonathan Druart 2021-05-03 09:23:38 UTC
(In reply to Victor Grousset/tuxayo from comment #23)
> >  c. Confirm that non linked subfields are stored and displayed correctly
> 
> - item edition list
> - MARC details
> - OPAC marc view
> 
> Is that it?

This patch set is affecting the add item form only.

> > d. Try with a "more subfield" that is linked with an authorised value
>  category
> 
> what is a "more subfield"?

A subfield that is not mapped with a DB column (and so stored in items.more_subfields_xml)

> > 3. Test barcode values
> 
> just put a barcode and using it afterwards?

barcode generated by a plugin
Comment 25 Victor Grousset/tuxayo 2021-06-02 19:01:44 UTC
error: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt).
Comment 26 Victor Grousset/tuxayo 2021-06-02 19:06:23 UTC
Test note:

Here is how to generate barcodes by a plugin:
> 952$p mapped to barcode.pl plugin in frameworks
> C4::Context->preference("autoBarcode");
> set autobarcode and then clicking in the field fro new item should generate

thanks kidclamp and cait :)
Comment 27 Jonathan Druart 2021-06-03 12:57:32 UTC
Created attachment 121589 [details] [review]
Bug 27526: Remove ModItemFromMarc from additem
Comment 28 Jonathan Druart 2021-06-03 12:57:37 UTC
Created attachment 121590 [details] [review]
Bug 27526: Remove AddItemFromMarc from additem
Comment 29 Jonathan Druart 2021-06-03 12:57:42 UTC
Created attachment 121591 [details] [review]
Bug 27526: Adjust code to use Koha::Items

Not that we removed all the transformations of the item and are using
Koha::Item from DB to TT (and the other way around), some code needs
adjustments.

- Retrieve host items can be simplified (see Koha::Biblio->host_items)
- Some TT variables have been renamed for better understanding
- Koha::Item->columns_to_str return a hashref with the representation
string of the columns. A date will return the value how it must be
displayed, using output_pref. A subfield linked with a AV will be
replaced with the AV's description.
- LastCreatedItem cookie serializes and stores Koha::Item->unblessed,
no longer the MARC::Record

Change in behaviour: If a subfield is linked with a AV cat and the value
is not a valid AV, before this patch the column was displayed with an
empty value. Now the column is hidden, it's considered empty.
In the sample data it happens with itemlost (0) and withdrawn (0).

Test plan:
1. Test the Prefill
 a. Turn PrefillItem on
 b. Fill in SubfieldsToUseWhenPrefill with some subfield codes
 c. Catalogue an item, save
 => The values from subfields listed in SubfieldsToUseWhenPrefill must be
 kept
2. more subfields
 a. Add subfields that are not linked with a koha field (k is available)
 b. Create an item and fill in all the values
 c. Confirm that non linked subfields are stored and displayed correctly
 d. Try with a "more subfield" that is linked with an authorised value
 category
3. Test barcode values
4. Test the different "Add" buttons at the bottom of the form
Comment 30 Marcel de Rooy 2021-06-18 09:27:31 UTC
   FAIL   pod coverage
                POD is missing for 'host_items'
   FAIL   pod coverage
                POD is missing for 'columns_to_str'
 FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt
   FAIL   filters
                missing_filter at line 174 (                    <input type="hidden" id="[%- mv.id | html -%]" name="[% kohafield %]" class="input_marceditor" size="50" maxlength="[%- mv.maxlength | html -%]" value="[%- mv.value | html -%]">)
                missing_filter at line 177 (                        <select name="[% kohafield %]" id="[%- mv.id | html -%]" class="input_marceditor" readonly="readonly" disabled="disabled">)
                missing_filter at line 179 (                        <select name="[% kohafield %]" id="[%- mv.id | html -%]" class="input_marceditor" data-category="[% mv.category | html %]">)
                missing_filter at line 195 (                        <input type="text" id="[%- mv.id | html -%]" name="[% kohafield %]" class="input_marceditor" size="50" maxlength="[%- mv.maxlength | html -%]" value="[%- mv.value | html -%]" readonly="readonly" />)
                missing_filter at line 197 (                        <input type="text" id="[%- mv.id | html -%]" name="[% kohafield %]" class="input_marceditor" size="50" maxlength="[%- mv.maxlength | html -%]" value="[%- mv.value | html -%]" />)
                missing_filter at line 203 (                        <input type="text" id="[%- mv.id | html -%]" name="[% kohafield %]" class="input_marceditor" size="50" maxlength="[%- mv.maxlength | html -%]" value="[%- mv.value | html -%]" readonly="readonly" />)
                missing_filter at line 205 (                        <input type="text" id="[%- mv.id | html -%]" name="[% kohafield %]" class="input_marceditor" size="50" maxlength="[%- mv.maxlength | html -%]" value="[%- mv.value | html -%]" />)
                missing_filter at line 215 (                        <input type="text" id="[%- mv.id | html -%]" name="[% kohafield %]" class="input_marceditor" size="50" maxlength="[%- mv.maxlength | html -%]" value="[%- mv.value | html -%]" readonly="readonly" />)
                missing_filter at line 217 (                        <input type="text" id="[%- mv.id | html -%]" name="[% kohafield %]" class="input_marceditor" size="50" maxlength="[%- mv.maxlength | html -%]" value="[%- mv.value | html -%]" />)
                missing_filter at line 221 (                        <textarea id="[%- mv.id | html -%]" name="[% kohafield %]" class="input_marceditor" rows="5" cols="64" readonly="readonly" >[% mv.value | html %]</textarea>)
                missing_filter at line 223 (                        <textarea id="[%- mv.id | html -%]" name="[% kohafield %]" class="input_marceditor" rows="5" cols="64" >[% mv.value | html %]</textarea>)
                missing_filter at line 228 (                    <input type="hidden" name="items.more_subfields_xml" value="[% subfield.subfield %]" />)
Comment 31 Jonathan Druart 2021-06-22 14:18:56 UTC
Created attachment 122288 [details] [review]
Bug 27526: Adjust code to use Koha::Items

Not that we removed all the transformations of the item and are using
Koha::Item from DB to TT (and the other way around), some code needs
adjustments.

- Retrieve host items can be simplified (see Koha::Biblio->host_items)
- Some TT variables have been renamed for better understanding
- Koha::Item->columns_to_str return a hashref with the representation
string of the columns. A date will return the value how it must be
displayed, using output_pref. A subfield linked with a AV will be
replaced with the AV's description.
- LastCreatedItem cookie serializes and stores Koha::Item->unblessed,
no longer the MARC::Record

Change in behaviour: If a subfield is linked with a AV cat and the value
is not a valid AV, before this patch the column was displayed with an
empty value. Now the column is hidden, it's considered empty.
In the sample data it happens with itemlost (0) and withdrawn (0).

Test plan:
1. Test the Prefill
 a. Turn PrefillItem on
 b. Fill in SubfieldsToUseWhenPrefill with some subfield codes
 c. Catalogue an item, save
 => The values from subfields listed in SubfieldsToUseWhenPrefill must be
 kept
2. more subfields
 a. Add subfields that are not linked with a koha field (k is available)
 b. Create an item and fill in all the values
 c. Confirm that non linked subfields are stored and displayed correctly
 d. Try with a "more subfield" that is linked with an authorised value
 category
3. Test barcode values
4. Test the different "Add" buttons at the bottom of the form
Comment 32 Jonathan Druart 2021-06-22 14:19:41 UTC
(In reply to Marcel de Rooy from comment #30)

Added the missing html filters.
Comment 33 Victor Grousset/tuxayo 2021-06-23 04:17:56 UTC
Found something very strange
- master
- no need to touch any setting
- add an item and fill "g - Cost, normal purchase price" with a number
- save
- apply patches & restart services
- refresh the add item page (additem.pl?biblionumber=295)
  (a refresh without resending the POST request)
  Template process failed: undef error - The given date (503.00) does not match the date format (iso)
- got back to master & restart services
- refresh the add item page (additem.pl?biblionumber=295)
- it works
Comment 34 Jonathan Druart 2021-06-23 08:36:56 UTC
Created attachment 122318 [details] [review]
Bug 27526: Improve grep for date fields

'replacementpricedate' should not catch 'price'
Comment 35 Jonathan Druart 2021-06-23 08:37:28 UTC
(In reply to Victor Grousset/tuxayo from comment #33)
> Found something very strange

Yes, silly error in the code. Should be fixed now.
Comment 36 Tomás Cohen Arazi 2021-07-07 21:20:24 UTC
Created attachment 122655 [details] [review]
Bug 27526: Remove ModItemFromMarc from additem

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 37 Tomás Cohen Arazi 2021-07-07 21:20:30 UTC
Created attachment 122656 [details] [review]
Bug 27526: Remove AddItemFromMarc from additem

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 38 Tomás Cohen Arazi 2021-07-07 21:20:36 UTC
Created attachment 122657 [details] [review]
Bug 27526: Adjust code to use Koha::Items

Not that we removed all the transformations of the item and are using
Koha::Item from DB to TT (and the other way around), some code needs
adjustments.

- Retrieve host items can be simplified (see Koha::Biblio->host_items)
- Some TT variables have been renamed for better understanding
- Koha::Item->columns_to_str return a hashref with the representation
string of the columns. A date will return the value how it must be
displayed, using output_pref. A subfield linked with a AV will be
replaced with the AV's description.
- LastCreatedItem cookie serializes and stores Koha::Item->unblessed,
no longer the MARC::Record

Change in behaviour: If a subfield is linked with a AV cat and the value
is not a valid AV, before this patch the column was displayed with an
empty value. Now the column is hidden, it's considered empty.
In the sample data it happens with itemlost (0) and withdrawn (0).

Test plan:
1. Test the Prefill
 a. Turn PrefillItem on
 b. Fill in SubfieldsToUseWhenPrefill with some subfield codes
 c. Catalogue an item, save
 => The values from subfields listed in SubfieldsToUseWhenPrefill must be
 kept
2. more subfields
 a. Add subfields that are not linked with a koha field (k is available)
 b. Create an item and fill in all the values
 c. Confirm that non linked subfields are stored and displayed correctly
 d. Try with a "more subfield" that is linked with an authorised value
 category
3. Test barcode values
4. Test the different "Add" buttons at the bottom of the form

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 39 Tomás Cohen Arazi 2021-07-07 21:20:41 UTC
Created attachment 122658 [details] [review]
Bug 27526: Improve grep for date fields

'replacementpricedate' should not catch 'price'

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 40 Tomás Cohen Arazi 2021-07-07 21:21:48 UTC
Needed a rebase.
Comment 41 Tomás Cohen Arazi 2021-07-08 12:55:26 UTC
I tested with this results:
- I added an extended attribute R, repeatable. Saved with values 3 and 4
=> FAIL: the MARC ended up with 3 | 3
- Marked another attribute as 'Important' (yellow message in the form, correct)
=> FAIL: Even if I choose a value it keeps saying I didn't fill it.
- Marked an attribute as 'Mandatory'
=> FAIL: It keeps saying I need to fill them
- AV-linked (including branches, csource, etc) field work correctly as before

So it looks good overall, only this small glitches.
Comment 42 Jonathan Druart 2021-07-08 13:53:11 UTC
Created attachment 122676 [details] [review]
Bug 27526: Fix repeatable more subfields

We retrieved always the first value.
Comment 43 Jonathan Druart 2021-07-08 13:53:16 UTC
Created attachment 122677 [details] [review]
Bug 27526: Fix mandatory and important checks

The input names have been changed from "field_value" to $kohafield.
Modifying this could have an impact in other area, where
CheckMandatorySubfields and CheckImportantSubfields are called.
Using .input_marceditor let us fix the additem.tt form and prevent to
break the other ones.
Note that the other ones are actually broken (!)

Also note that there is a typo in the error message
        alertString2 += "\n- " + "%s " + MSG_MANDATORY_FIELDS_EMPTY.format(total_errors);
There is an extra %s
Comment 44 Jonathan Druart 2021-07-08 13:59:04 UTC
(In reply to Jonathan Druart from comment #43)
> Note that the other ones are actually broken (!)

Opened bug 28690.

> Also note that there is a typo in the error message
>         alertString2 += "\n- " + "%s " +
> MSG_MANDATORY_FIELDS_EMPTY.format(total_errors);
> There is an extra %s

Opened bug 28689.
Comment 45 Tomás Cohen Arazi 2021-07-08 14:44:52 UTC
Created attachment 122681 [details] [review]
Bug 27526: Fix repeatable more subfields

We retrieved always the first value.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 46 Tomás Cohen Arazi 2021-07-08 14:44:59 UTC
Created attachment 122682 [details] [review]
Bug 27526: Fix mandatory and important checks

The input names have been changed from "field_value" to $kohafield.
Modifying this could have an impact in other area, where
CheckMandatorySubfields and CheckImportantSubfields are called.
Using .input_marceditor let us fix the additem.tt form and prevent to
break the other ones.
Note that the other ones are actually broken (!)

Also note that there is a typo in the error message
        alertString2 += "\n- " + "%s " + MSG_MANDATORY_FIELDS_EMPTY.format(total_errors);
There is an extra %s

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 47 Tomás Cohen Arazi 2021-07-08 14:45:52 UTC
Nice work!
Comment 48 Victor Grousset/tuxayo 2021-07-08 15:17:24 UTC
Hi :)
Issue found.

master
enable prefill on e f g h
add an item, next item e f g h are prefilled
apply patch, restart
go back to edit item page
(not prefilled)
add an item, next item isn't prefilled so it's confirmed prefill doesn't work

now something less on topic that I really don't understand:
rollback code to master
restart all service
go back to edit item page
Can't call method "field" on unblessed reference at /kohadevbox/koha/cataloguing/additem.pl line 978

How can this happen? There isn't a DB upgrade so rollbacking the code and restarting should be enough, right?
Anyway, resetting all data allowed to retry on master and confirm that it works
Comment 49 Tomás Cohen Arazi 2021-07-08 15:34:22 UTC
(In reply to Victor Grousset/tuxayo from comment #48)
> 
> now something less on topic that I really don't understand:
> rollback code to master
> restart all service
> go back to edit item page
> Can't call method "field" on unblessed reference at
> /kohadevbox/koha/cataloguing/additem.pl line 978
> 
> How can this happen? There isn't a DB upgrade so rollbacking the code and
> restarting should be enough, right?

Can you try cleaning the cookies?
Comment 50 Victor Grousset/tuxayo 2021-07-08 16:05:51 UTC
(In reply to Tomás Cohen Arazi from comment #49)
> Can you try cleaning the cookies?

Indeed it was that, thanks! (tried in private browsing)
Mystery solved about this secondary testing issue.
That's scary that something like that can happen and persist even after a logout.
(but a server side reset solves it o_O )
Comment 51 Jonathan Druart 2021-07-09 08:04:45 UTC
Created attachment 122694 [details] [review]
Bug 27526: Fix PrefillItem

We are basically adding:
  $current_item = $item->unblessed;

Other changes are for readability
Comment 52 Jonathan Druart 2021-07-09 08:04:50 UTC
Created attachment 122695 [details] [review]
Bug 27526: Fix Add & duplicate
Comment 53 Jonathan Druart 2021-07-09 08:04:55 UTC
Created attachment 122696 [details] [review]
Bug 27526: Fix Add multiple copies

This has been moved to Koha::Item->store by bug 27545.
Comment 54 Jonathan Druart 2021-07-09 08:07:25 UTC
Good catch Victor!

I found other issues as well. It would be great to have somebody familiar with the different options (auto barcode, duplicate copies, prefillitem, etc.) test those follow-ups.

Nick or Andrew maybe? Unless you are, Victor!
Comment 55 Victor Grousset/tuxayo 2021-07-10 02:33:12 UTC
(In reply to Jonathan Druart from comment #54)
> Unless you are, Victor!

Nope, I'm learning everything there!
Really, for main 4 things of the test plan I had to ask or dig a tutorial for each one ^^"
Comment 56 Jonathan Druart 2021-07-19 14:50:32 UTC
Created attachment 122946 [details] [review]
Bug 27526: Remove ModItemFromMarc from additem

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 57 Jonathan Druart 2021-07-19 14:50:38 UTC
Created attachment 122947 [details] [review]
Bug 27526: Remove AddItemFromMarc from additem

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 58 Jonathan Druart 2021-07-19 14:50:44 UTC
Created attachment 122948 [details] [review]
Bug 27526: Adjust code to use Koha::Items

Not that we removed all the transformations of the item and are using
Koha::Item from DB to TT (and the other way around), some code needs
adjustments.

- Retrieve host items can be simplified (see Koha::Biblio->host_items)
- Some TT variables have been renamed for better understanding
- Koha::Item->columns_to_str return a hashref with the representation
string of the columns. A date will return the value how it must be
displayed, using output_pref. A subfield linked with a AV will be
replaced with the AV's description.
- LastCreatedItem cookie serializes and stores Koha::Item->unblessed,
no longer the MARC::Record

Change in behaviour: If a subfield is linked with a AV cat and the value
is not a valid AV, before this patch the column was displayed with an
empty value. Now the column is hidden, it's considered empty.
In the sample data it happens with itemlost (0) and withdrawn (0).

Test plan:
1. Test the Prefill
 a. Turn PrefillItem on
 b. Fill in SubfieldsToUseWhenPrefill with some subfield codes
 c. Catalogue an item, save
 => The values from subfields listed in SubfieldsToUseWhenPrefill must be
 kept
2. more subfields
 a. Add subfields that are not linked with a koha field (k is available)
 b. Create an item and fill in all the values
 c. Confirm that non linked subfields are stored and displayed correctly
 d. Try with a "more subfield" that is linked with an authorised value
 category
3. Test barcode values
4. Test the different "Add" buttons at the bottom of the form

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 59 Jonathan Druart 2021-07-19 14:50:48 UTC
Created attachment 122949 [details] [review]
Bug 27526: Improve grep for date fields

'replacementpricedate' should not catch 'price'

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 60 Jonathan Druart 2021-07-19 14:50:54 UTC
Created attachment 122950 [details] [review]
Bug 27526: Fix repeatable more subfields

We retrieved always the first value.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 61 Jonathan Druart 2021-07-19 14:50:59 UTC
Created attachment 122951 [details] [review]
Bug 27526: Fix mandatory and important checks

The input names have been changed from "field_value" to $kohafield.
Modifying this could have an impact in other area, where
CheckMandatorySubfields and CheckImportantSubfields are called.
Using .input_marceditor let us fix the additem.tt form and prevent to
break the other ones.
Note that the other ones are actually broken (!)

Also note that there is a typo in the error message
        alertString2 += "\n- " + "%s " + MSG_MANDATORY_FIELDS_EMPTY.format(total_errors);
There is an extra %s

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 62 Jonathan Druart 2021-07-19 14:51:07 UTC
Created attachment 122952 [details] [review]
Bug 27526: Fix PrefillItem

We are basically adding:
  $current_item = $item->unblessed;

Other changes are for readability
Comment 63 Jonathan Druart 2021-07-19 14:51:13 UTC
Created attachment 122953 [details] [review]
Bug 27526: Fix Add & duplicate
Comment 64 Jonathan Druart 2021-07-19 14:51:18 UTC
Created attachment 122954 [details] [review]
Bug 27526: Fix Add multiple copies

This has been moved to Koha::Item->store by bug 27545.
Comment 65 Jonathan Druart 2021-07-19 14:52:01 UTC
Patches have been rebased (easy conflicts with bug 17600).
Comment 66 Jonathan Druart 2021-07-20 09:45:51 UTC
Created attachment 122957 [details] [review]
Bug 27526: Fix incorrect condition

The barcode was always prefilled!
Comment 67 Jonathan Druart 2021-07-20 10:26:00 UTC
Created attachment 122958 [details] [review]
Bug 27526: Fix incorrect condition

The barcode was always prefilled!
Comment 68 Jonathan Druart 2021-07-20 10:26:06 UTC
Created attachment 122959 [details] [review]
Bug 27526: Fix empty string vs undef

Empty strings must be removed, not inserted as empty strings in DB.
The relevant code is in TransformHtmlToXml, $skip variable.
Comment 69 Jonathan Druart 2021-07-20 10:34:58 UTC
Note that after this patchset the "Total Checkouts" column displays "0" instead of an empty string. It is a bugfix IMO: items.issues has 0 in DB (before and after this patchset).
Comment 70 Jonathan Druart 2021-07-20 12:51:28 UTC
Created attachment 122974 [details] [review]
Bug 27526: Remove uneeded call to TransformMarcToKoha

And also clean some imports.
Comment 71 Jonathan Druart 2021-07-21 08:30:32 UTC
Created attachment 123006 [details] [review]
Bug 27526: Remove uneeded call to TransformMarcToKoha

And also clean some imports.
Comment 72 Jonathan Druart 2021-07-21 09:57:36 UTC
Created attachment 123007 [details] [review]
Bug 27526: Fix encoding issue on subfield

If you have a "é" subfield it should work!

Note that VARCHAR(1) for binary means 1-byte (from MySQL doc):
"For example, if the default character set is utf8mb4, CHAR(5) BINARY is
treated as CHAR(5) CHARACTER SET utf8mb4 COLLATE utf8mb4_bin. This
differs from BINARY(5), which stores 5-byte binary strings that have the
binary character set and collation."
Comment 73 Jonathan Druart 2021-07-23 13:17:37 UTC
Created attachment 123110 [details] [review]
Bug 27526: Fix cn_source display

For an unknown reason C4::Biblio::GetAuthorisedValueDesc (that we are
calling from Koha::Item->columns_to_str) does not deal with class
sources.
Comment 74 Joonas Kylmälä 2021-07-23 14:07:00 UTC
This should be need sign-off?
Comment 75 Jonathan Druart 2021-07-23 14:18:34 UTC
(In reply to Joonas Kylmälä from comment #74)
> This should be need sign-off?

I am building a lot of things on top of this and fixing bugs when I find them.

A strong QA review would be good.
Comment 76 Joonas Kylmälä 2021-07-30 10:35:18 UTC
- Koha::Biblio::host_items and Koha::Item::columns_to_str need POD and tests. The additem.pl doesn't load because "The method Koha::Biblio->host_items is not covered by tests!" so testing cannot be done currently.

- I noticed a lot of bug fixes outside the scope of just removing Mod/AddItemFromMarc would be nice if those could be splitted to separate patches that don't even depend on this, also same goes with other patches having multiple changes in them that are not intertwined. I would be hesitant to merge these without detailed explanations for every individual change but not blocking in case other QA team member wants to approve these.
Comment 77 Jonathan Druart 2021-07-30 14:24:49 UTC
(In reply to Joonas Kylmälä from comment #76)

> - Koha::Biblio::host_items and Koha::Item::columns_to_str need POD and
> tests. The additem.pl doesn't load because "The method
> Koha::Biblio->host_items is not covered by tests!" so testing cannot be done
> currently.

It's working for me, looks like you forgot to restart plack.

Koha/Biblio.pm:sub host_items {
cataloguing/additem.pl:for my $item ( $biblio->items->as_list, $biblio->host_items->as_list ) {

> - I noticed a lot of bug fixes outside the scope of just removing
> Mod/AddItemFromMarc would be nice if those could be splitted to separate
> patches that don't even depend on this, also same goes with other patches
> having multiple changes in them that are not intertwined. I would be
> hesitant to merge these without detailed explanations for every individual
> change but not blocking in case other QA team member wants to approve these.

I don't really see how I could have split more, the patches have quite isolated changes. I am even suggesting you to review the before/after additem side-by-side.
That will be easier to catch potential issues.

That being said, I have been working several days on top of those patches and I am pretty confident in them. I am only expected very hard to catch regressions, that are, most of the time, never caught during SO or QA step.
Comment 78 Jonathan Druart 2021-07-30 14:26:09 UTC
Created attachment 123329 [details] [review]
Bug 27526: Add missing POD
Comment 79 Jonathan Druart 2021-07-30 14:27:28 UTC
I will provide the missing tests next week. Thanks for the reminder!
Comment 80 Jonathan Druart 2021-08-02 12:47:38 UTC
Created attachment 123372 [details] [review]
Bug 27526: Add tests for columns_to_str and host_items
Comment 81 Jonathan Druart 2021-08-02 12:48:14 UTC
(In reply to Jonathan Druart from comment #79)
> I will provide the missing tests next week. Thanks for the reminder!

Done!
And I found a bug, so tests++ :)
Comment 82 Jonathan Druart 2021-08-03 12:30:34 UTC
Created attachment 123415 [details] [review]
Bug 27526: Add tests for columns_to_str and host_items
Comment 83 Jonathan Druart 2021-08-03 12:30:51 UTC
txn_begin was missing.