Bug 27526

Summary: Remove Mod/AddItemFromMarc from additem.pl
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Tomás Cohen Arazi <tomascohen>
Severity: enhancement    
Priority: P5 - low CC: andrewfh, dcook, jean-manuel.broust, joonas.kylmala, julian.maurice, kyle, m.de.rooy, martin.renvoize, nick, nugged, tomascohen, victor
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27837
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28689
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28690
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28694
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28783
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28832
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29633
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00
Bug Depends on: 8976, 27545    
Bug Blocks: 28687, 34689, 26351, 28445, 28694, 28769, 29111, 29496, 29662, 29962, 30178, 30717, 30909, 31179    
Attachments: Bug 27526: Remove ModItemFromMarc from additem
Bug 27526: Remove AddItemFromMarc from additem
Bug 27526: Adjust code to use Koha::Items
Bug 27526: Remove ModItemFromMarc from additem
Bug 27526: Remove AddItemFromMarc from additem
Bug 27526: Adjust code to use Koha::Items
Bug 27526: Remove ModItemFromMarc from additem
Bug 27526: Remove AddItemFromMarc from additem
Bug 27526: Adjust code to use Koha::Items
Bug 27526: Adjust code to use Koha::Items
Bug 27526: Improve grep for date fields
Bug 27526: Remove ModItemFromMarc from additem
Bug 27526: Remove AddItemFromMarc from additem
Bug 27526: Adjust code to use Koha::Items
Bug 27526: Improve grep for date fields
Bug 27526: Fix repeatable more subfields
Bug 27526: Fix mandatory and important checks
Bug 27526: Fix repeatable more subfields
Bug 27526: Fix mandatory and important checks
Bug 27526: Fix PrefillItem
Bug 27526: Fix Add & duplicate
Bug 27526: Fix Add multiple copies
Bug 27526: Remove ModItemFromMarc from additem
Bug 27526: Remove AddItemFromMarc from additem
Bug 27526: Adjust code to use Koha::Items
Bug 27526: Improve grep for date fields
Bug 27526: Fix repeatable more subfields
Bug 27526: Fix mandatory and important checks
Bug 27526: Fix PrefillItem
Bug 27526: Fix Add & duplicate
Bug 27526: Fix Add multiple copies
Bug 27526: Fix incorrect condition
Bug 27526: Fix incorrect condition
Bug 27526: Fix empty string vs undef
Bug 27526: Remove uneeded call to TransformMarcToKoha
Bug 27526: Remove uneeded call to TransformMarcToKoha
Bug 27526: Fix encoding issue on subfield
Bug 27526: Fix cn_source display
Bug 27526: Add missing POD
Bug 27526: Add tests for columns_to_str and host_items
Bug 27526: Add tests for columns_to_str and host_items
Bug 27526: Remove ModItemFromMarc from additem
Bug 27526: Remove AddItemFromMarc from additem
Bug 27526: Adjust code to use Koha::Items
Bug 27526: Improve grep for date fields
Bug 27526: Fix repeatable more subfields
Bug 27526: Fix mandatory and important checks
Bug 27526: Fix PrefillItem
Bug 27526: Fix Add & duplicate
Bug 27526: Fix Add multiple copies
Bug 27526: Fix incorrect condition
Bug 27526: Fix empty string vs undef
Bug 27526: Remove uneeded call to TransformMarcToKoha
Bug 27526: Fix encoding issue on subfield
Bug 27526: Fix cn_source display
Bug 27526: Add missing POD
Bug 27526: Add tests for columns_to_str and host_items
Bug 27526: Add tests for columns_to_str and host_items
Bug 27526: Remove ModItemFromMarc from additem
Bug 27526: Remove AddItemFromMarc from additem
Bug 27526: Adjust code to use Koha::Items
Bug 27526: Improve grep for date fields
Bug 27526: Fix repeatable more subfields
Bug 27526: Fix mandatory and important checks
Bug 27526: Fix PrefillItem
Bug 27526: Fix Add & duplicate
Bug 27526: Fix Add multiple copies
Bug 27526: Fix incorrect condition
Bug 27526: Fix empty string vs undef
Bug 27526: Remove uneeded call to TransformMarcToKoha
Bug 27526: Fix encoding issue on subfield
Bug 27526: Fix cn_source display
Bug 27526: Add missing POD
Bug 27526: Add tests for columns_to_str and host_items
Bug 27526: Fix perlcritic in tests
Bug 27526: (QA follow-up) Wrap tests in transaction
Bug 27526: Improve robustness of tests
Bug 27526: Correct NULL vs empty string when editing
Bug 27526: (follow-up) Remove uneeded call to TransformMarcToKoha
Bug 27526: Fix 'duplicate item'
Bug 27526: Fix 'add & duplicate item'
Bug 27526: Fix for prefill preventing duplication
Bug 27526: Fix for prefill preventing duplication 2
Bug 27526: Fix SubfieldsToUseWhenPrefill

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.
Comment 84 Martin Renvoize 2021-08-11 07:55:18 UTC
Created attachment 123757 [details] [review]
Bug 27526: Remove ModItemFromMarc from additem

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 85 Martin Renvoize 2021-08-11 07:55:22 UTC
Created attachment 123758 [details] [review]
Bug 27526: Remove AddItemFromMarc from additem

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 86 Martin Renvoize 2021-08-11 07:55:27 UTC
Created attachment 123759 [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>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 87 Martin Renvoize 2021-08-11 07:55:32 UTC
Created attachment 123760 [details] [review]
Bug 27526: Improve grep for date fields

'replacementpricedate' should not catch 'price'

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 88 Martin Renvoize 2021-08-11 07:55:36 UTC
Created attachment 123761 [details] [review]
Bug 27526: Fix repeatable more subfields

We retrieved always the first value.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 89 Martin Renvoize 2021-08-11 07:55:41 UTC
Created attachment 123762 [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>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 90 Martin Renvoize 2021-08-11 07:55:46 UTC
Created attachment 123763 [details] [review]
Bug 27526: Fix PrefillItem

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

Other changes are for readability

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 91 Martin Renvoize 2021-08-11 07:55:52 UTC
Created attachment 123764 [details] [review]
Bug 27526: Fix Add & duplicate

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 92 Martin Renvoize 2021-08-11 07:55:58 UTC
Created attachment 123765 [details] [review]
Bug 27526: Fix Add multiple copies

This has been moved to Koha::Item->store by bug 27545.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 93 Martin Renvoize 2021-08-11 07:56:04 UTC
Created attachment 123766 [details] [review]
Bug 27526: Fix incorrect condition

The barcode was always prefilled!

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 94 Martin Renvoize 2021-08-11 07:56:09 UTC
Created attachment 123767 [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.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 95 Martin Renvoize 2021-08-11 07:56:14 UTC
Created attachment 123768 [details] [review]
Bug 27526: Remove uneeded call to TransformMarcToKoha

And also clean some imports.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 96 Martin Renvoize 2021-08-11 07:56:18 UTC
Created attachment 123769 [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."

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 97 Martin Renvoize 2021-08-11 07:56:24 UTC
Created attachment 123770 [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.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 98 Martin Renvoize 2021-08-11 07:56:29 UTC
Created attachment 123771 [details] [review]
Bug 27526: Add missing POD

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 99 Martin Renvoize 2021-08-11 07:56:34 UTC
Created attachment 123772 [details] [review]
Bug 27526: Add tests for columns_to_str and host_items

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 100 Martin Renvoize 2021-08-11 07:58:51 UTC
I tested pretty thoroughly but will admit this isn't an area I'm most familiar with.

Added my SO line to all the patches.
Comment 101 Jonathan Druart 2021-09-20 14:48:11 UTC
Created attachment 125069 [details] [review]
Bug 27526: Add tests for columns_to_str and host_items
Comment 102 Tomás Cohen Arazi 2021-10-01 11:32:05 UTC
Created attachment 125605 [details] [review]
Bug 27526: Remove ModItemFromMarc from additem

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 103 Tomás Cohen Arazi 2021-10-01 11:32:13 UTC
Created attachment 125606 [details] [review]
Bug 27526: Remove AddItemFromMarc from additem

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 104 Tomás Cohen Arazi 2021-10-01 11:32:23 UTC
Created attachment 125607 [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>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 105 Tomás Cohen Arazi 2021-10-01 11:32:31 UTC
Created attachment 125608 [details] [review]
Bug 27526: Improve grep for date fields

'replacementpricedate' should not catch 'price'

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 106 Tomás Cohen Arazi 2021-10-01 11:32:41 UTC
Created attachment 125609 [details] [review]
Bug 27526: Fix repeatable more subfields

We retrieved always the first value.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 107 Tomás Cohen Arazi 2021-10-01 11:32:51 UTC
Created attachment 125610 [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>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 108 Tomás Cohen Arazi 2021-10-01 11:33:01 UTC
Created attachment 125611 [details] [review]
Bug 27526: Fix PrefillItem

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

Other changes are for readability

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 109 Tomás Cohen Arazi 2021-10-01 11:33:15 UTC
Created attachment 125612 [details] [review]
Bug 27526: Fix Add & duplicate

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 110 Tomás Cohen Arazi 2021-10-01 11:33:22 UTC
Created attachment 125613 [details] [review]
Bug 27526: Fix Add multiple copies

This has been moved to Koha::Item->store by bug 27545.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 111 Tomás Cohen Arazi 2021-10-01 11:33:29 UTC
Created attachment 125614 [details] [review]
Bug 27526: Fix incorrect condition

The barcode was always prefilled!

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 112 Tomás Cohen Arazi 2021-10-01 11:33:37 UTC
Created attachment 125615 [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.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 113 Tomás Cohen Arazi 2021-10-01 11:33:45 UTC
Created attachment 125616 [details] [review]
Bug 27526: Remove uneeded call to TransformMarcToKoha

And also clean some imports.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 114 Tomás Cohen Arazi 2021-10-01 11:33:53 UTC
Created attachment 125617 [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."

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 115 Tomás Cohen Arazi 2021-10-01 11:34:02 UTC
Created attachment 125618 [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.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 116 Tomás Cohen Arazi 2021-10-01 11:34:10 UTC
Created attachment 125619 [details] [review]
Bug 27526: Add missing POD

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 117 Tomás Cohen Arazi 2021-10-01 11:34:16 UTC
Created attachment 125620 [details] [review]
Bug 27526: Add tests for columns_to_str and host_items

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 118 Tomás Cohen Arazi 2021-10-01 11:37:25 UTC
Ok, I already tested and signed this before.
Tests were added as requested and they pass.
All found issues by testers addressed in follow-up patches.

No QA issues.
Comment 119 Martin Renvoize 2021-10-06 08:02:43 UTC
Great to see this one PQA :)
Comment 120 Jonathan Druart 2021-10-06 12:57:04 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 121 Jonathan Druart 2021-10-06 13:56:13 UTC
Created attachment 125818 [details] [review]
Bug 27526: Fix perlcritic in tests

Problem while critiquing "t/db_dependent/Koha/Item.t": Can't parse code: Encountered unexpected character '195'

non-ascii chars must be quoted when used as hash keys
Comment 122 Tomás Cohen Arazi 2021-10-06 16:44:58 UTC
Created attachment 125839 [details] [review]
Bug 27526: (QA follow-up) Wrap tests in transaction

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 123 Jonathan Druart 2021-10-07 08:54:37 UTC
Created attachment 125863 [details] [review]
Bug 27526: 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
Comment 124 Jonathan Druart 2021-10-07 09:20:58 UTC
Last 3 patches pushed to master.
Comment 125 Jonathan Druart 2021-10-11 09:12:58 UTC
Created attachment 126034 [details] [review]
Bug 27526: Correct NULL vs empty string when editing

When an item is edit we must keep the NULL values in DB if the input
have been left empty.
It also fixes the "barcode cannot be unique" error when an item does not
have a barcode.
Comment 126 Jonathan Druart 2021-10-11 09:13:53 UTC
Nick, Tomas, can you have a close look at this patch please?
Comment 127 Marcel de Rooy 2021-10-11 09:20:31 UTC
(In reply to Jonathan Druart from comment #125)
> When an item is edit we must keep the NULL values in DB if the input
> have been left empty.

I saw that before? It is presented as a fact, but I am not sure about that. When the user saved the form and left a few fields empty, you could argue that empty string reflects this 'choice', while NULL says that we just dont know.
Comment 128 Jonathan Druart 2021-10-11 12:15:34 UTC
(In reply to Marcel de Rooy from comment #127)
> (In reply to Jonathan Druart from comment #125)
> > When an item is edit we must keep the NULL values in DB if the input
> > have been left empty.
> 
> I saw that before? It is presented as a fact, but I am not sure about that.
> When the user saved the form and left a few fields empty, you could argue
> that empty string reflects this 'choice', while NULL says that we just dont
> know.

We use NULL when creating, we should keep it when editing IMO.
Comment 129 Jonathan Druart 2021-10-12 06:55:20 UTC
Created attachment 126051 [details] [review]
Bug 27526: (follow-up) Remove uneeded call to TransformMarcToKoha
Comment 130 Jonathan Druart 2021-10-12 07:14:09 UTC
(In reply to Jonathan Druart from comment #129)
> Created attachment 126051 [details] [review] [review]
> Bug 27526: (follow-up) Remove uneeded call to TransformMarcToKoha

Patch pushed to master.
Comment 131 Jonathan Druart 2021-10-13 07:29:29 UTC
Created attachment 126143 [details] [review]
Bug 27526: Fix 'duplicate item'
Comment 132 Jonathan Druart 2021-10-13 07:34:49 UTC
Created attachment 126144 [details] [review]
Bug 27526: Fix 'add & duplicate item'
Comment 133 Andrii Nugged 2021-10-16 09:27:43 UTC
Created attachment 126393 [details] [review]
Bug 27526: Fix for prefill preventing duplication
Comment 134 Andrii Nugged 2021-10-16 09:28:43 UTC
JD: we still "hotfixing" so I adding a link to the chain with your hotfixes, this one above made it working at last on our live server.
Comment 135 Jonathan Druart 2021-10-18 09:34:03 UTC
Last 3 patches pushed to master!
Comment 136 Andrii Nugged 2021-10-26 15:43:49 UTC
(In reply to Jonathan Druart from comment #125)
> Created attachment 126034 [details] [review] [review]
> Bug 27526: Correct NULL vs empty string when editing

looks like we lost this one from master.

Jonathan: for a reason?
Comment 137 Jonathan Druart 2021-10-27 07:11:29 UTC
(In reply to Andrew Nugged from comment #136)
> (In reply to Jonathan Druart from comment #125)
> > Created attachment 126034 [details] [review] [review] [review]
> > Bug 27526: Correct NULL vs empty string when editing
> 
> looks like we lost this one from master.
> 
> Jonathan: for a reason?

Oops, sorry, I forgot it!
Comment 138 Jonathan Druart 2021-10-27 07:26:47 UTC
I was actually waiting for confirmation, I haven't flagged it as pushed.

It's now pushed.
Comment 139 Andrii Nugged 2021-10-27 09:31:58 UTC
thanks! Now I am doing my build for piloting library, production, let's see what else pops-up! :)
Comment 140 Andrii Nugged 2021-10-28 05:59:22 UTC
Created attachment 127025 [details] [review]
Bug 27526: Fix for prefill preventing duplication 2

Prefill feature should not be in the priority
when user choose "Duplicate" for the item
Comment 141 Jonathan Druart 2021-10-28 08:52:41 UTC
Created attachment 127032 [details] [review]
Bug 27526: Fix SubfieldsToUseWhenPrefill

my $a = "z";
my @x = split ( ' ', $a ) || ("");

@x will be [1]

Which is not at all what we are expecting here!
Be more verbose and don't introduce bug.
Comment 142 Jonathan Druart 2021-10-28 09:11:22 UTC
Last two patches pushed to master.