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.
Created attachment 115679 [details] [review] Bug 27526: Remove ModItemFromMarc from additem
Created attachment 115680 [details] [review] Bug 27526: Remove AddItemFromMarc from additem
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.
I would happily test these, this code is confusing and due for an overhaul
Count me in for testing/review too.. it would be a lovely area to clean up.
I expected to see Koha::Item->new_from_marc({ })
(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.
(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.
(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?
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
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.
+ 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?
(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. ^^
Well, that's not what I meant. *almost* completely :D But yes, we could move them to a new DB table.
(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!
Can I get some testing on this please?
Created attachment 117806 [details] [review] Bug 27526: Remove ModItemFromMarc from additem
Created attachment 117807 [details] [review] Bug 27526: Remove AddItemFromMarc from additem
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
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?
(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.
(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.
> 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?
(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
error: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt).
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 :)
Created attachment 121589 [details] [review] Bug 27526: Remove ModItemFromMarc from additem
Created attachment 121590 [details] [review] Bug 27526: Remove AddItemFromMarc from additem
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
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 %]" />)
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
(In reply to Marcel de Rooy from comment #30) Added the missing html filters.
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
Created attachment 122318 [details] [review] Bug 27526: Improve grep for date fields 'replacementpricedate' should not catch 'price'
(In reply to Victor Grousset/tuxayo from comment #33) > Found something very strange Yes, silly error in the code. Should be fixed now.
Created attachment 122655 [details] [review] Bug 27526: Remove ModItemFromMarc from additem Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 122656 [details] [review] Bug 27526: Remove AddItemFromMarc from additem Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
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>
Needed a rebase.
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.
Created attachment 122676 [details] [review] Bug 27526: Fix repeatable more subfields We retrieved always the first value.
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
(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.
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>
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>
Nice work!
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
(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?
(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 )
Created attachment 122694 [details] [review] Bug 27526: Fix PrefillItem We are basically adding: $current_item = $item->unblessed; Other changes are for readability
Created attachment 122695 [details] [review] Bug 27526: Fix Add & duplicate
Created attachment 122696 [details] [review] Bug 27526: Fix Add multiple copies This has been moved to Koha::Item->store by bug 27545.
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!
(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 ^^"
Created attachment 122946 [details] [review] Bug 27526: Remove ModItemFromMarc from additem Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 122947 [details] [review] Bug 27526: Remove AddItemFromMarc from additem Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
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>
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>
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>
Created attachment 122952 [details] [review] Bug 27526: Fix PrefillItem We are basically adding: $current_item = $item->unblessed; Other changes are for readability
Created attachment 122953 [details] [review] Bug 27526: Fix Add & duplicate
Created attachment 122954 [details] [review] Bug 27526: Fix Add multiple copies This has been moved to Koha::Item->store by bug 27545.
Patches have been rebased (easy conflicts with bug 17600).
Created attachment 122957 [details] [review] Bug 27526: Fix incorrect condition The barcode was always prefilled!
Created attachment 122958 [details] [review] Bug 27526: Fix incorrect condition The barcode was always prefilled!
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.
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).
Created attachment 122974 [details] [review] Bug 27526: Remove uneeded call to TransformMarcToKoha And also clean some imports.
Created attachment 123006 [details] [review] Bug 27526: Remove uneeded call to TransformMarcToKoha And also clean some imports.
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."
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.
This should be need sign-off?
(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.
- 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.
(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.
Created attachment 123329 [details] [review] Bug 27526: Add missing POD
I will provide the missing tests next week. Thanks for the reminder!
Created attachment 123372 [details] [review] Bug 27526: Add tests for columns_to_str and host_items
(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++ :)
Created attachment 123415 [details] [review] Bug 27526: Add tests for columns_to_str and host_items
txn_begin was missing.
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>
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>
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>
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>
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>
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>
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>
Created attachment 123764 [details] [review] Bug 27526: Fix Add & duplicate Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
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>
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>
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>
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>
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>
Created attachment 123771 [details] [review] Bug 27526: Add missing POD Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
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.
Created attachment 125069 [details] [review] Bug 27526: Add tests for columns_to_str and host_items
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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.
Great to see this one PQA :)
Pushed to master for 21.11, thanks to everybody involved!
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
Created attachment 125839 [details] [review] Bug 27526: (QA follow-up) Wrap tests in transaction Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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
Last 3 patches pushed to master.
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.
Nick, Tomas, can you have a close look at this patch please?
(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.
(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.
Created attachment 126051 [details] [review] Bug 27526: (follow-up) Remove uneeded call to TransformMarcToKoha
(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.
Created attachment 126143 [details] [review] Bug 27526: Fix 'duplicate item'
Created attachment 126144 [details] [review] Bug 27526: Fix 'add & duplicate item'
Created attachment 126393 [details] [review] Bug 27526: Fix for prefill preventing duplication
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.
Last 3 patches pushed to master!
(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?
(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!
I was actually waiting for confirmation, I haven't flagged it as pushed. It's now pushed.
thanks! Now I am doing my build for piloting library, production, let's see what else pops-up! :)
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
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.
Last two patches pushed to master.