Bug 20817 - Add ability to retain 9xx fields when adding on order item to existing record
Summary: Add ability to retain 9xx fields when adding on order item to existing record
Status: ASSIGNED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nick Clemens
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-05-24 17:16 UTC by Kyle M Hall
Modified: 2023-08-03 16:21 UTC (History)
16 users (show)

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


Attachments
Bug 20817 - Add ability to retain 9xx fields when adding on order item to existing record (6.37 KB, patch)
2018-10-03 13:15 UTC, Devinim
Details | Diff | Splinter Review
Bug 20817 - Add ability to retain 9xx fields when adding on order item to existing record (16.00 KB, patch)
2018-10-12 16:10 UTC, Devinim
Details | Diff | Splinter Review
Bug 20817: Add ability to retain 9xx fields when adding on order item to existing record (16.61 KB, patch)
2019-03-18 08:19 UTC, Devinim
Details | Diff | Splinter Review
Bug 20817: Add ability to retain 9xx fields when adding on order item to existing record (16.71 KB, patch)
2019-03-19 14:47 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 20817: Add ability to retain 9xx fields when adding on order item to existing record (16.70 KB, patch)
2020-05-21 15:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Move subroutines to a new Perl module (19.30 KB, patch)
2020-05-21 15:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Add unit tests (6.90 KB, patch)
2020-05-21 15:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Rename get_infos_syspref_on_item to GetMarcItemFieldsToOrderValues (5.51 KB, patch)
2020-05-21 15:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Rename get_infos_syspref to GetMarcFieldsToOrderValues (4.69 KB, patch)
2020-05-21 15:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Remove syspref from parameters, it is invariant (5.89 KB, patch)
2020-05-21 15:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Fix QA script issues (2.74 KB, patch)
2021-01-19 18:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Add ability to retain 9xx fields when adding on order item to existing record (16.68 KB, patch)
2021-03-17 13:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Move subroutines to a new Perl module (19.26 KB, patch)
2021-03-17 13:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Add unit tests (6.90 KB, patch)
2021-03-17 13:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Rename get_infos_syspref_on_item to GetMarcItemFieldsToOrderValues (5.51 KB, patch)
2021-03-17 13:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Rename get_infos_syspref to GetMarcFieldsToOrderValues (4.69 KB, patch)
2021-03-17 13:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Remove syspref from parameters, it is invariant (5.89 KB, patch)
2021-03-17 13:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Fix QA script issues (2.33 KB, patch)
2021-03-17 13:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Update module to use YAML::XS (1.34 KB, patch)
2021-03-17 13:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: (QA follow-up) Remove size=1 attribute (1.51 KB, patch)
2021-07-09 07:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20817: _trim should always return a value (652 bytes, patch)
2021-07-13 19:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: (QA follow-up) Fix 'undefined subroutine' error (1.35 KB, patch)
2021-07-14 14:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: (QA follow-up) Add missing module Output (917 bytes, patch)
2021-07-15 12:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20817: (QA follow-up) Fix _trim (958 bytes, patch)
2021-07-15 13:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20817: Add ability to retain 9xx fields when adding on order item to existing record (16.62 KB, patch)
2021-12-20 16:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Move subroutines to a new Perl module (18.68 KB, patch)
2021-12-20 16:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Add unit tests (6.88 KB, patch)
2021-12-20 16:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Rename get_infos_syspref_on_item to GetMarcItemFieldsToOrderValues (5.50 KB, patch)
2021-12-20 16:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Rename get_infos_syspref to GetMarcFieldsToOrderValues (4.68 KB, patch)
2021-12-20 16:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Remove syspref from parameters, it is invariant (5.88 KB, patch)
2021-12-20 16:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Fix QA script issues (2.31 KB, patch)
2021-12-20 16:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Update module to use YAML::XS (1.32 KB, patch)
2021-12-20 16:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: (QA follow-up) Remove size=1 attribute (1.47 KB, patch)
2021-12-20 16:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: _trim should always return a value (638 bytes, patch)
2021-12-20 16:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: (QA follow-up) Fix 'undefined subroutine' error (1.34 KB, patch)
2021-12-20 16:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: (QA follow-up) Fix _trim (920 bytes, patch)
2021-12-20 16:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Fix incorrect namespace for subroutine call (1.26 KB, patch)
2021-12-20 17:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Import C4::Items::PrepareItemrecordDisplay for neworderempty.pl (811 bytes, patch)
2021-12-20 17:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Add ability to retain 9xx fields when adding on order item to existing record (16.67 KB, patch)
2021-12-20 18:47 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 20817: Move subroutines to a new Perl module (18.74 KB, patch)
2021-12-20 18:47 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 20817: Add unit tests (6.95 KB, patch)
2021-12-20 18:48 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 20817: Rename get_infos_syspref_on_item to GetMarcItemFieldsToOrderValues (5.56 KB, patch)
2021-12-20 18:48 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 20817: Rename get_infos_syspref to GetMarcFieldsToOrderValues (4.74 KB, patch)
2021-12-20 18:48 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 20817: Remove syspref from parameters, it is invariant (5.94 KB, patch)
2021-12-20 18:48 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 20817: Fix QA script issues (2.38 KB, patch)
2021-12-20 18:48 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 20817: Update module to use YAML::XS (1.38 KB, patch)
2021-12-20 18:48 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 20817: (QA follow-up) Remove size=1 attribute (1.53 KB, patch)
2021-12-20 18:48 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 20817: _trim should always return a value (702 bytes, patch)
2021-12-20 18:48 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 20817: (QA follow-up) Fix 'undefined subroutine' error (1.40 KB, patch)
2021-12-20 18:48 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 20817: (QA follow-up) Fix _trim (985 bytes, patch)
2021-12-20 18:48 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 20817: Fix incorrect namespace for subroutine call (1.32 KB, patch)
2021-12-20 18:48 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 20817: Import C4::Items::PrepareItemrecordDisplay for neworderempty.pl (875 bytes, patch)
2021-12-20 18:48 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 20817: (QA follow-up) Add new parameter for PrepareItemrecordDisplay (2.62 KB, patch)
2022-03-15 15:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: (QA follow-up) Fix POD for GetMarcFieldsToOrderValues and GetMarcItemFieldsToOrderValues (2.08 KB, patch)
2022-03-15 15:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Add ability to retain 9xx fields when adding on order item to existing record (17.11 KB, patch)
2022-03-15 15:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Move subroutines to a new Perl module (18.74 KB, patch)
2022-03-15 15:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Add unit tests (6.95 KB, patch)
2022-03-15 15:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Rename get_infos_syspref_on_item to GetMarcItemFieldsToOrderValues (5.56 KB, patch)
2022-03-15 15:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Rename get_infos_syspref to GetMarcFieldsToOrderValues (4.74 KB, patch)
2022-03-15 15:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Remove syspref from parameters, it is invariant (5.94 KB, patch)
2022-03-15 15:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Fix QA script issues (2.38 KB, patch)
2022-03-15 15:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Update module to use YAML::XS (1.38 KB, patch)
2022-03-15 15:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: (QA follow-up) Remove size=1 attribute (1.53 KB, patch)
2022-03-15 15:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: _trim should always return a value (702 bytes, patch)
2022-03-15 15:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: (QA follow-up) Fix 'undefined subroutine' error (1.40 KB, patch)
2022-03-15 15:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: (QA follow-up) Fix _trim (985 bytes, patch)
2022-03-15 15:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Fix incorrect namespace for subroutine call (1.32 KB, patch)
2022-03-15 15:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Import C4::Items::PrepareItemrecordDisplay for neworderempty.pl (875 bytes, patch)
2022-03-15 15:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: (QA follow-up) Add new parameter for PrepareItemrecordDisplay (2.62 KB, patch)
2022-03-15 15:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: (QA follow-up) Fix POD for GetMarcFieldsToOrderValues and GetMarcItemFieldsToOrderValues (2.08 KB, patch)
2022-03-15 15:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Add ability to retain 9xx fields when adding on order item to existing record (16.61 KB, patch)
2022-10-25 16:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Move subroutines to a new Perl module (18.65 KB, patch)
2022-10-25 16:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Add unit tests (6.88 KB, patch)
2022-10-25 16:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Rename get_infos_syspref_on_item to GetMarcItemFieldsToOrderValues (5.49 KB, patch)
2022-10-25 16:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Rename get_infos_syspref to GetMarcFieldsToOrderValues (4.58 KB, patch)
2022-10-25 16:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Remove syspref from parameters, it is invariant (5.79 KB, patch)
2022-10-25 16:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Fix QA script issues (2.31 KB, patch)
2022-10-25 16:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Update module to use YAML::XS (1.32 KB, patch)
2022-10-25 16:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: (QA follow-up) Remove size=1 attribute (1.47 KB, patch)
2022-10-25 16:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: _trim should always return a value (636 bytes, patch)
2022-10-25 16:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: (QA follow-up) Fix 'undefined subroutine' error (1.34 KB, patch)
2022-10-25 16:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: (QA follow-up) Fix _trim (918 bytes, patch)
2022-10-25 16:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Fix incorrect namespace for subroutine call (1.26 KB, patch)
2022-10-25 16:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Import C4::Items::PrepareItemrecordDisplay for neworderempty.pl (809 bytes, patch)
2022-10-25 16:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: (QA follow-up) Add new parameter for PrepareItemrecordDisplay (2.62 KB, patch)
2022-10-25 16:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: (QA follow-up) Fix POD for GetMarcFieldsToOrderValues and GetMarcItemFieldsToOrderValues (2.08 KB, patch)
2022-10-25 16:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Add ability to retain 9xx fields when adding on order item to existing record (16.61 KB, patch)
2022-10-25 16:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Squashed follow-ups (29.70 KB, patch)
2022-10-25 16:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: (QA follow-up) Remove size=1 attribute (1.47 KB, patch)
2022-10-25 16:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: (QA follow-up) Fix _trim (918 bytes, patch)
2022-10-25 16:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Add ability to retain 9xx fields when adding on order item to existing record (34.03 KB, patch)
2022-10-25 16:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Add ability to retain 9xx fields when adding on order item to existing record (34.87 KB, patch)
2022-10-25 16:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20817: Add ability to retain 9xx fields when adding on order item to existing record (35.56 KB, patch)
2022-10-31 12:16 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2018-05-24 17:16:23 UTC
Acquisitions: attaching orders to existing records while retaining 9xx information.
Comment 1 Katrin Fischer 2018-05-24 19:41:15 UTC
Hi Kyle, can you explain this one a bit more?
Comment 2 Kyle M Hall 2018-05-31 12:36:49 UTC
We would like an Acquisitions development that would streamline the workflow of ordering items that already exist in the system (reorders).

The ideal workflow would be something along the lines of:
1. Staged files are added to a basket and a duplicate warning is received. You click "Display them" (This step already exists)
2. From this screen, when you click "add order" on an item, you are able to add that order record to an existing bib record using the information in the 9xx fields. Currently this step discards any 9xx information and creates a blank order record.
3. Click "Save" and move on to the next duplicate.

Since the ability to add an order to an existing record is already present, we're just looking to add the 9xx information as well.
Comment 3 Katrin Fischer 2018-06-01 05:40:28 UTC
Thx for the explanation!
Comment 4 Devinim 2018-10-03 13:15:40 UTC
Created attachment 79867 [details] [review]
Bug 20817 - Add ability to retain 9xx fields when adding on order item to existing record

To test:
1) Choose an existing record from "Add order to basket" area either searching a record or clicking "From a staged file" section
2) After you reach "New order" page for existing record you will see new fieldset called "Existing items" which are existing items belong to a record not previously ordered
3) When you choose item(s) from this new tab fields of "Accounting details" tab will be automatically filled
4) When you finished click save button.
Comment 5 Devinim 2018-10-12 16:10:27 UTC
Created attachment 80537 [details] [review]
Bug 20817 - Add ability to retain 9xx fields when adding on order item to existing record

Test plan:

1 - Edit the MarcItemFieldsToOrder and MarcFieldsToOrder syspref. For example:
MarcItemFieldsToOrder
homebranch: 975$a
holdingbranch: 975$b
itype: 975$c
nonpublic_note: 975$d
public_note: 975$e
loc: 975$f
ccode: 975$g
price: 975$h
replacementprice: 975$i
itemcallnumber: 975$j
notforloan: 975$m
uri: 975$n
copyno: 975$o


MarcFieldsToOrder
price: 975$h

2 - Stage your order file from vendor. (Test file : https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=57715 )
3 - Add to basket from staged file
4 - You are notified of duplicates
5 - View duplicates and choose the existing record to order from
6 - The values in the staged file should be used to create the new item(s) (we should not have to set quantity,price, etc)
Comment 6 Liz Rea 2019-03-01 18:51:23 UTC
Hi Devinim,

Thanks for looking at this feature, it will be really cool.

This works well populating item values from a staged record. I don't particularly like the UX of adding the two items (the "add item" button is hard to spot in the long list of fields), but that isn't related to this patch. 

There are some QA fails here: 

 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/acqui/neworderempty.tt
   FAIL	  filters
		missing_filter at line 328 (                                <input type="hidden" name="tag" value="[% iteminfo.tag %]" />)
		missing_filter at line 309 (                                    <label class="required">[% iteminfo.subfield %] - [% iteminfo.marc_lib %]</label>)
		missing_filter at line 777 (    [% jsinclude # Parse the page template's JavaScript block if necessary %])
		missing_filter at line 329 (                                <input type="hidden" name="subfield" value="[% iteminfo.subfield %]" />)
		missing_filter at line 330 (                                <input type="hidden" name="mandatory" value="[% iteminfo.mandatory %]" />)
		missing_filter at line 311 (                                    <label>[% iteminfo.subfield %] - [% iteminfo.marc_lib %]</label>)
		missing_filter at line 307 (                            <div class="subfield_line" id="subfield[% iteminfo.serialid %][% iteminfo.countitems %][% iteminfo.subfield %][% iteminfo.random %]">)
		missing_filter at line 319 (                                                <option value="[% value %]">[% iteminfo.marc_value.labels.$value %]</option>)
		missing_filter at line 326 (                                <input type="hidden" name="itemid" value="[% itemID %]" />)
		missing_filter at line 324 (                                    [% iteminfo.marc_value %])
		missing_filter at line 327 (                                <input type="hidden" name="kohafield" value="[% iteminfo.kohafield %]" />)
		missing_filter at line 337 (                        <input class="addItemControl" name="buttonPlus" style="cursor:pointer; margin:0 1em;" onclick="addItem(this,'[% UniqueItemFields %]')" value="Add item" type="button">)
		missing_filter at line 299 (                <div id="itemblock[% itemID %]" >)
		missing_filter at line 317 (                                                <option value="[% value %]" selected="selected">[% iteminfo.marc_value.labels.$value %]</option>)
		missing_filter at line 342 (                            <input class="addItemControl" name="buttonAddMulti&quot;" style="cursor:pointer; margin:0 1em;" onclick="checkCount( this ,'[% UniqueItemFields %]')" value="Add" type="button">)
		missing_filter at line 303 (                                <li style="[% iteminfo.hidden %];">)


* Commit title does not start with 'Bug XXXXX: ' - 034220c0cc

Fix those up and I'll sign off on this for you. :)

Cheers,
Liz
Comment 7 Devinim 2019-03-18 08:19:14 UTC
Created attachment 86711 [details] [review]
Bug 20817: Add ability to retain 9xx fields when adding on order item to existing record
Comment 8 Liz Rea 2019-03-19 14:47:20 UTC
Created attachment 86746 [details] [review]
Bug 20817: Add ability to retain 9xx fields when adding on order item to existing record

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

https://bugs.koha-community.org/show_bug.cgi?id=20187
Comment 9 Katrin Fischer 2019-05-11 14:08:17 UTC
The amount of code added to neworderempty.pl makes me uneasy here. It adds 5 subs to neworderempty.pl. 

Some of it is copy & paste from addorderiso2709.pl (get_infos_syspref_on_item, equal_number_of_fields...)

I think this needs more work and centralizing the code in question as part of it, so it can be reused in a maintainable way.
Comment 10 Devinim 2019-05-13 08:12:09 UTC
Hi Katrin,

We discussed copy & paste issue before sending this patch. Since there is not any other usage of get_infos_syspref_on_item other than the addorderiso2709.pl we decided to leave as is and copy & paste in the new development.

We were not reach any rule for coding guidelines for that situation. Do we need to write a new module if a sub will be used in different code? What should be  the exact amount of usage to move subs in a module? 2,3,4

Thanks.
Comment 11 Katrin Fischer 2019-05-13 10:56:40 UTC
Hi, my opinion would be to not reuse subs (copy and paste) them at all. There should be only one place to maintain the code that does a specific thing. Others opinions might differ? I am adding a few more people to the bug in hope to get some insight.
Comment 12 Jonathan Druart 2019-05-13 13:32:29 UTC
(In reply to Devinim from comment #10)
> We were not reach any rule for coding guidelines for that situation. Do we
> need to write a new module if a sub will be used in different code? What
> should be  the exact amount of usage to move subs in a module? 2,3,4

Hi, you are not supposed to c/p code. If you want to reuse bad code (in .pl and not tested), you will need to move it to a pm, try to provide tests, then reuse it.
Comment 13 Katrin Fischer 2019-06-20 14:12:19 UTC
Moving to failed QA, hope this can be fixed!
Comment 14 Kyle M Hall 2020-05-21 15:12:25 UTC
Created attachment 105214 [details] [review]
Bug 20817: Add ability to retain 9xx fields when adding on order item to existing record

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

https://bugs.koha-community.org/show_bug.cgi?id=20187
Comment 15 Kyle M Hall 2020-05-21 15:12:35 UTC
Created attachment 105215 [details] [review]
Bug 20817: Move subroutines to a new Perl module
Comment 16 Kyle M Hall 2020-05-21 15:12:37 UTC
Created attachment 105216 [details] [review]
Bug 20817: Add unit tests
Comment 17 Kyle M Hall 2020-05-21 15:12:40 UTC
Created attachment 105217 [details] [review]
Bug 20817: Rename get_infos_syspref_on_item to GetMarcItemFieldsToOrderValues
Comment 18 Kyle M Hall 2020-05-21 15:12:43 UTC
Created attachment 105218 [details] [review]
Bug 20817: Rename get_infos_syspref to GetMarcFieldsToOrderValues
Comment 19 Kyle M Hall 2020-05-21 15:12:46 UTC
Created attachment 105219 [details] [review]
Bug 20817: Remove syspref from parameters, it is invariant
Comment 20 Katrin Fischer 2020-07-05 01:17:12 UTC
Please fix!

 FAIL	acqui/neworderempty.pl
   FAIL	  valid
		syntax error 
		acqui/neworderempty.pl had compilation errors.

 FAIL	Koha/Acquisition/Utils.pm
   FAIL	  pod
		*** WARNING: 
		=head3 without preceding higher level
		 
		 in file Koha/Acquisition/Utils.pm
		
		*** WARNING: 
		=head3 without preceding higher level
		 
		 in file Koha/Acquisition/Utils.pm
		
		*** WARNING: 
		=head3 without preceding higher level
		 
		 in file Koha/Acquisition/Utils.pm
Comment 21 Martha Fuerst 2021-01-19 15:14:28 UTC
Making noise here - this is something that we very much need fixed, as it is how our vendor gets BISAC information from our orders for the branches in our system using BISAC.

Thank you!

Marti Fuerst
Huntsville-Madison County Public Library
Comment 22 Kyle M Hall 2021-01-19 18:02:12 UTC
Created attachment 115343 [details] [review]
Bug 20817: Fix QA script issues
Comment 23 Martha Fuerst 2021-03-10 17:27:55 UTC
Thanks, Kyle!

We are really looking forward to getting this fixed.
Comment 24 Tomás Cohen Arazi 2021-03-17 11:38:00 UTC
Willing to do a QA round on this one. Can you please rebase?
Comment 25 Kyle M Hall 2021-03-17 13:46:20 UTC
Created attachment 118371 [details] [review]
Bug 20817: Add ability to retain 9xx fields when adding on order item to existing record

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

https://bugs.koha-community.org/show_bug.cgi?id=20187
Comment 26 Kyle M Hall 2021-03-17 13:46:28 UTC
Created attachment 118372 [details] [review]
Bug 20817: Move subroutines to a new Perl module
Comment 27 Kyle M Hall 2021-03-17 13:46:31 UTC
Created attachment 118373 [details] [review]
Bug 20817: Add unit tests
Comment 28 Kyle M Hall 2021-03-17 13:46:34 UTC
Created attachment 118374 [details] [review]
Bug 20817: Rename get_infos_syspref_on_item to GetMarcItemFieldsToOrderValues
Comment 29 Kyle M Hall 2021-03-17 13:46:37 UTC
Created attachment 118375 [details] [review]
Bug 20817: Rename get_infos_syspref to GetMarcFieldsToOrderValues
Comment 30 Kyle M Hall 2021-03-17 13:46:40 UTC
Created attachment 118376 [details] [review]
Bug 20817: Remove syspref from parameters, it is invariant
Comment 31 Kyle M Hall 2021-03-17 13:46:43 UTC
Created attachment 118377 [details] [review]
Bug 20817: Fix QA script issues
Comment 32 Kyle M Hall 2021-03-17 13:46:46 UTC
Created attachment 118378 [details] [review]
Bug 20817: Update module to use YAML::XS
Comment 33 Marcel de Rooy 2021-07-09 06:48:59 UTC
Looking here
Comment 34 Marcel de Rooy 2021-07-09 06:49:41 UTC
 FAIL   acqui/neworderempty.pl
   FAIL   valid
                Useless use of a variable in void context
Comment 35 Marcel de Rooy 2021-07-09 06:57:24 UTC
(In reply to Marcel de Rooy from comment #34)
>  FAIL   acqui/neworderempty.pl
>    FAIL   valid
>                 Useless use of a variable in void context

Without a line number, this is hard to find. I checked perlcritic and it gives a lot of noise on neworderempty.pl (depending on level), but this warn I did not find..
Comment 36 Marcel de Rooy 2021-07-09 07:05:54 UTC
 FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/acqui/neworderempty.tt
   FAIL   forbidden patterns
                forbidden pattern: Don't use select tag's size attribute where it is 1 (bug 28066) (line 345)
Comment 37 Marcel de Rooy 2021-07-09 07:06:17 UTC
Created attachment 122692 [details] [review]
Bug 20817: (QA follow-up) Remove size=1 attribute

Resolves QA warning, with reference to bug 28066.
Line 345 of acqui/neworderempty.tt
Comment 38 Marcel de Rooy 2021-07-09 07:16:49 UTC
(In reply to Kyle M Hall from comment #25)
> Created attachment 118371 [details] [review] [review]
> Bug 20817: Add ability to retain 9xx fields when adding on order item to
> existing record
> 
> Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Remarkably, we lost the test plan here ;)
Comment 39 Marcel de Rooy 2021-07-09 07:35:47 UTC
Hmm. I am sorry to note but it crashes already at the first stage of testing this. When we still have this kind of typo in the code now, I would recommend adding an updated test plan and setting back to Needs Signoff. It does not help me trust these changes.

Undefined subroutine &C4::Acquisition::Utils::GetMarcItemFieldsToOrderValues called at /usr/share/koha/acqui/addorderiso2709.pl line 540
my $alliteminfos = C4::Acquisition::Utils::GetMarcItemFieldsToOrderValues( $marcrecord, [ 'homebranch', 'holdingbranch', 'itype', 'nonpublic_note', 'public_note', 'loc', 'ccode', 'notforloan', 'uri', 'copyno', 'price', 'replacementprice', 'itemcallnumber', 'quantity', 'budget_code' ] );

=> Koha::Acquisition::Utils etc
Comment 40 Marcel de Rooy 2021-07-09 08:33:14 UTC
(In reply to Marcel de Rooy from comment #35)
> (In reply to Marcel de Rooy from comment #34)
> >  FAIL   acqui/neworderempty.pl
> >    FAIL   valid
> >                 Useless use of a variable in void context
> 
> Without a line number, this is hard to find. I checked perlcritic and it
> gives a lot of noise on neworderempty.pl (depending on level), but this warn
> I did not find..

perl -cw acqui/neworderempty.pl
Useless use of a variable in void context at acqui/neworderempty.pl line 700.

Hmm line 700 is }
;)
Comment 41 Marcel de Rooy 2021-07-09 08:39:08 UTC
            my %itemrecord=(
                'homebranch' => _trim( $iteminfos->{homebranch} ),
                'holdingbranch' => _trim( $iteminfos->{holdingbranch} ),

If you look at _trim, you see that it may return undef or empty list in list context. And that would produce odd elements in hash kind of things as in the above lines.
Comment 42 Kyle M Hall 2021-07-13 19:16:50 UTC
Created attachment 122834 [details] [review]
Bug 20817: _trim should always return a value
Comment 43 Marcel de Rooy 2021-07-14 11:05:29 UTC
     my $string = shift;
     return unless $string;
     $string =~ s/^\s+|\s+$//g;
-    return $string;
+    return $string // q{};

Note the first return too.
Comment 44 Marcel de Rooy 2021-07-14 11:05:55 UTC
(In reply to Marcel de Rooy from comment #39)
> Hmm. I am sorry to note but it crashes already at the first stage of testing
> this. When we still have this kind of typo in the code now, I would
> recommend adding an updated test plan and setting back to Needs Signoff. It
> does not help me trust these changes.
> 
> Undefined subroutine &C4::Acquisition::Utils::GetMarcItemFieldsToOrderValues
> called at /usr/share/koha/acqui/addorderiso2709.pl line 540
> my $alliteminfos = C4::Acquisition::Utils::GetMarcItemFieldsToOrderValues(
> $marcrecord, [ 'homebranch', 'holdingbranch', 'itype', 'nonpublic_note',
> 'public_note', 'loc', 'ccode', 'notforloan', 'uri', 'copyno', 'price',
> 'replacementprice', 'itemcallnumber', 'quantity', 'budget_code' ] );
> 
> => Koha::Acquisition::Utils etc

This one is more serious
Comment 45 Kyle M Hall 2021-07-14 14:32:25 UTC
Created attachment 122841 [details] [review]
Bug 20817: (QA follow-up) Fix 'undefined subroutine' error
Comment 46 Marcel de Rooy 2021-07-15 11:58:44 UTC
Having another look here
Comment 47 Marcel de Rooy 2021-07-15 12:46:10 UTC
To clear:
Useless use of a variable in void context at acqui/neworderempty.pl line 700.

You need to look at line 492
output_html_with_http_headers $input, $cookie, $template->output;

And then understand that the C4::Output module was not included yet at the start of the file.
Hmm ;)
Adding a follow-up.
Comment 48 Marcel de Rooy 2021-07-15 12:48:09 UTC
Created attachment 122858 [details] [review]
Bug 20817: (QA follow-up) Add missing module Output

This resolves a perl -cw warning:
Useless use of a variable in void context at acqui/neworderempty.pl line 700.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 49 Marcel de Rooy 2021-07-15 13:45:35 UTC
Created attachment 122861 [details] [review]
Bug 20817: (QA follow-up) Fix _trim

The patch 'trim should always return a value' does not completely
do the job yet. And might have unwanted effects for the '0' case.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 50 Marcel de Rooy 2021-07-15 13:46:31 UTC
acqui/neworderempty.pl:    my ($marcrecord, $encoding) = MARCfindbreeding($params->{'breedingid'});
acqui/neworderempty.pl:=head2 MARCfindbreeding
acqui/neworderempty.pl:  $record = MARCfindbreeding($breedingid);
acqui/neworderempty.pl:sub MARCfindbreeding {
acqui/neworderempty.pl:    my ($marcrecord, $encoding) = MARCfindbreeding($breedingid);
authorities/authorities.pl:sub MARCfindbreeding_auth {
authorities/authorities.pl:    ( $record, $encoding ) = MARCfindbreeding_auth( $breedingid );
cataloguing/addbiblio.pl:=head2 MARCfindbreeding
cataloguing/addbiblio.pl:    $record = MARCfindbreeding($breedingid);
cataloguing/addbiblio.pl:sub MARCfindbreeding {
cataloguing/addbiblio.pl:    ( $record, $encoding ) = MARCfindbreeding( $breedingid ) ;
=> Can we consolidate this in a module ?

Do we need a (small) extra test for the new itemrecord parameter in PrepareItemrecordDisplay ?

Another crash:
my $alliteminfos = C4::Acquisition::Utils::GetMarcItemFieldsToOrderValues( $marcrecord, [ 'homebranch', 'holdingbranch', 'itype', 'nonpublic_note', 'public_note', 'loc', 'ccode', 'notforloan', 'uri', 'copyno', 'price', 'replacementprice', 'itemcallnumber', 'quantity', 'budget_code' ] );

Test plan
The whole workflow is not completely clear to me here. We should have a solid test plan that tells me how I can verify it this development really works as we hope it does.
With the changes and time passed, a new signoff from someone with Acquisition experience would be welcome.
Comment 51 Jonathan Druart 2021-07-15 14:14:47 UTC
"Util" sounds wrong, it's a too generic term IMO.
Comment 52 Kyle M Hall 2021-07-15 15:31:14 UTC
(In reply to Jonathan Druart from comment #51)
> "Util" sounds wrong, it's a too generic term IMO.

Choose a replacement word and I'll make it happen!
Comment 53 David Cook 2021-07-18 23:59:52 UTC
(In reply to Kyle M Hall from comment #52)
> (In reply to Jonathan Druart from comment #51)
> > "Util" sounds wrong, it's a too generic term IMO.
> 
> Choose a replacement word and I'll make it happen!

Looking at Nick's email "Helper/Static methods for objects", I'm curious why we'd create a Util module and not just use static/class methods from the relevant Koha::Acquisitions namespace (probably Koha::Acquisition::Orders in this case?). 

I had a similar dilemma the other day on a different project. I almost made a Utils module, but then I realized that would just fragment my application model more, so I added a static/class method, and kept everything together.
Comment 54 Tomás Cohen Arazi 2021-07-28 15:38:13 UTC
Hi, get_infos_syspref() should be using C4::Context->yaml_preference instead of manually dealing with YAML.
Comment 55 Kyle M Hall 2021-12-20 16:58:00 UTC
Created attachment 128766 [details] [review]
Bug 20817: Add ability to retain 9xx fields when adding on order item to existing record

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

https://bugs.koha-community.org/show_bug.cgi?id=20187
Comment 56 Kyle M Hall 2021-12-20 16:58:18 UTC
Created attachment 128767 [details] [review]
Bug 20817: Move subroutines to a new Perl module
Comment 57 Kyle M Hall 2021-12-20 16:58:21 UTC
Created attachment 128768 [details] [review]
Bug 20817: Add unit tests
Comment 58 Kyle M Hall 2021-12-20 16:58:25 UTC
Created attachment 128769 [details] [review]
Bug 20817: Rename get_infos_syspref_on_item to GetMarcItemFieldsToOrderValues
Comment 59 Kyle M Hall 2021-12-20 16:58:29 UTC
Created attachment 128770 [details] [review]
Bug 20817: Rename get_infos_syspref to GetMarcFieldsToOrderValues
Comment 60 Kyle M Hall 2021-12-20 16:58:33 UTC
Created attachment 128771 [details] [review]
Bug 20817: Remove syspref from parameters, it is invariant
Comment 61 Kyle M Hall 2021-12-20 16:58:37 UTC
Created attachment 128772 [details] [review]
Bug 20817: Fix QA script issues
Comment 62 Kyle M Hall 2021-12-20 16:58:41 UTC
Created attachment 128773 [details] [review]
Bug 20817: Update module to use YAML::XS
Comment 63 Kyle M Hall 2021-12-20 16:58:45 UTC
Created attachment 128774 [details] [review]
Bug 20817: (QA follow-up) Remove size=1 attribute

Resolves QA warning, with reference to bug 28066.
Line 345 of acqui/neworderempty.tt
Comment 64 Kyle M Hall 2021-12-20 16:58:49 UTC
Created attachment 128775 [details] [review]
Bug 20817: _trim should always return a value
Comment 65 Kyle M Hall 2021-12-20 16:58:53 UTC
Created attachment 128776 [details] [review]
Bug 20817: (QA follow-up) Fix 'undefined subroutine' error
Comment 66 Kyle M Hall 2021-12-20 16:58:57 UTC
Created attachment 128777 [details] [review]
Bug 20817: (QA follow-up) Fix _trim

The patch 'trim should always return a value' does not completely
do the job yet. And might have unwanted effects for the '0' case.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 67 Andrew Fuerste-Henry 2021-12-20 17:29:14 UTC
I'm getting an error trying to test this. With the patch applied, I get a duplicate warning when trying to submit my order, click "Use existing" from the dupe check page, and get this error: 
Undefined subroutine &C4::Acquisition::Utils::GetMarcItemFieldsToOrderValues called at /kohadevbox/koha/acqui/neworderempty.pl line 652
Comment 68 Kyle M Hall 2021-12-20 17:45:41 UTC
Created attachment 128782 [details] [review]
Bug 20817: Fix incorrect namespace for subroutine call
Comment 69 Kyle M Hall 2021-12-20 17:56:19 UTC
Created attachment 128783 [details] [review]
Bug 20817: Import C4::Items::PrepareItemrecordDisplay for neworderempty.pl
Comment 70 Andrew Fuerste-Henry 2021-12-20 18:47:52 UTC
Created attachment 128785 [details] [review]
Bug 20817: Add ability to retain 9xx fields when adding on order item to existing record

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

https://bugs.koha-community.org/show_bug.cgi?id=20187

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 71 Andrew Fuerste-Henry 2021-12-20 18:47:56 UTC
Created attachment 128786 [details] [review]
Bug 20817: Move subroutines to a new Perl module

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 72 Andrew Fuerste-Henry 2021-12-20 18:48:00 UTC
Created attachment 128787 [details] [review]
Bug 20817: Add unit tests

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 73 Andrew Fuerste-Henry 2021-12-20 18:48:04 UTC
Created attachment 128788 [details] [review]
Bug 20817: Rename get_infos_syspref_on_item to GetMarcItemFieldsToOrderValues

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 74 Andrew Fuerste-Henry 2021-12-20 18:48:09 UTC
Created attachment 128789 [details] [review]
Bug 20817: Rename get_infos_syspref to GetMarcFieldsToOrderValues

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 75 Andrew Fuerste-Henry 2021-12-20 18:48:13 UTC
Created attachment 128790 [details] [review]
Bug 20817: Remove syspref from parameters, it is invariant

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 76 Andrew Fuerste-Henry 2021-12-20 18:48:17 UTC
Created attachment 128791 [details] [review]
Bug 20817: Fix QA script issues

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 77 Andrew Fuerste-Henry 2021-12-20 18:48:22 UTC
Created attachment 128792 [details] [review]
Bug 20817: Update module to use YAML::XS

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 78 Andrew Fuerste-Henry 2021-12-20 18:48:26 UTC
Created attachment 128793 [details] [review]
Bug 20817: (QA follow-up) Remove size=1 attribute

Resolves QA warning, with reference to bug 28066.
Line 345 of acqui/neworderempty.tt

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 79 Andrew Fuerste-Henry 2021-12-20 18:48:32 UTC
Created attachment 128794 [details] [review]
Bug 20817: _trim should always return a value

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 80 Andrew Fuerste-Henry 2021-12-20 18:48:38 UTC
Created attachment 128795 [details] [review]
Bug 20817: (QA follow-up) Fix 'undefined subroutine' error

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 81 Andrew Fuerste-Henry 2021-12-20 18:48:44 UTC
Created attachment 128796 [details] [review]
Bug 20817: (QA follow-up) Fix _trim

The patch 'trim should always return a value' does not completely
do the job yet. And might have unwanted effects for the '0' case.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 82 Andrew Fuerste-Henry 2021-12-20 18:48:49 UTC
Created attachment 128797 [details] [review]
Bug 20817: Fix incorrect namespace for subroutine call

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 83 Andrew Fuerste-Henry 2021-12-20 18:48:54 UTC
Created attachment 128798 [details] [review]
Bug 20817: Import C4::Items::PrepareItemrecordDisplay for neworderempty.pl

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 84 Katrin Fischer 2022-03-14 22:33:22 UTC
1) Can you please explain this change in Items.pm? 

This would also affect the item form in serials IIRC.

@@ -1559,6 +1559,8 @@ sub PrepareItemrecordDisplay {
     my $itemrecord;
     if ($itemnum) {
         $itemrecord = C4::Items::GetMarcItem( $bibnum, $itemnum );
+    }elsif ($defaultvalues && $defaultvalues->{'itemrecord'} ) {
+        $itemrecord = $defaultvalues->{'itemrecord'};
     }

2) GetMarcFieldsToOrderValues in Koha/Acquisition/Utils.pm

* Method names in Koha should be using snake case.
* Is it possible that the $syspref parameter has been removed/not implemented? It appears to be hardcoded to Koha/Acquisition/Utils.pm.

+=head3 GetMarcFieldsToOrderValues($syspref_name, $record, $field_list)
+
+my $data = Koha::Acquisition::Utils::GetMarcFieldsToOrderValues('MarcFieldsToOrder', $marcrecord, ['price', 'quantity', 'budget_code', etc.]);
+

+sub GetMarcFieldsToOrderValues {
+    my ($record, $field_list) = @_;
+    my $syspref = C4::Context->preference('MarcFieldsToOrder');

3) GetMarcItemFieldsToOrderValues in Koha/Acquisition/Utils.pm

Same questions as for 2) actually.

4) Test plan

None of the commit messages contain much of a description or test plan. I found this - is it still accurate? Could you add some notes for how GetMarcFieldsToOrder and GetMarcItemFieldsToOrder should be set up/tested? I haven't used these features so far.

To test:
1) Choose an existing record from "Add order to basket" area either searching a record or clicking "From a staged file" section
2) After you reach "New order" page for existing record you will see new fieldset called "Existing items" which are existing items belong to a record not previously ordered
3) When you choose item(s) from this new tab fields of "Accounting details" tab will be automatically filled
4) When you finished click save button.

If you could at least enhance the first commit message that would be great.
Comment 85 Kyle M Hall 2022-03-15 15:39:27 UTC
Created attachment 131732 [details] [review]
Bug 20817: (QA follow-up) Add new parameter for PrepareItemrecordDisplay
Comment 86 Kyle M Hall 2022-03-15 15:39:39 UTC
Created attachment 131733 [details] [review]
Bug 20817: (QA follow-up) Fix POD for GetMarcFieldsToOrderValues and GetMarcItemFieldsToOrderValues
Comment 87 Kyle M Hall 2022-03-15 15:50:12 UTC
Created attachment 131734 [details] [review]
Bug 20817: Add ability to retain 9xx fields when adding on order item to existing record

1) Choose an existing record from "Add order to basket" area either searching a record or clicking "From a staged file" section
2) After you reach "New order" page for existing record you will see new fieldset called "Existing items" which are existing items belong to a record not previously ordered
3) When you choose item(s) from this new tab fields of "Accounting details" tab will be automatically filled
4) When you finished click save button.

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

https://bugs.koha-community.org/show_bug.cgi?id=20187

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 88 Kyle M Hall 2022-03-15 15:50:24 UTC
Created attachment 131735 [details] [review]
Bug 20817: Move subroutines to a new Perl module

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 89 Kyle M Hall 2022-03-15 15:50:29 UTC
Created attachment 131736 [details] [review]
Bug 20817: Add unit tests

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 90 Kyle M Hall 2022-03-15 15:50:33 UTC
Created attachment 131737 [details] [review]
Bug 20817: Rename get_infos_syspref_on_item to GetMarcItemFieldsToOrderValues

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 91 Kyle M Hall 2022-03-15 15:50:38 UTC
Created attachment 131738 [details] [review]
Bug 20817: Rename get_infos_syspref to GetMarcFieldsToOrderValues

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 92 Kyle M Hall 2022-03-15 15:50:43 UTC
Created attachment 131739 [details] [review]
Bug 20817: Remove syspref from parameters, it is invariant

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 93 Kyle M Hall 2022-03-15 15:50:47 UTC
Created attachment 131740 [details] [review]
Bug 20817: Fix QA script issues

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 94 Kyle M Hall 2022-03-15 15:50:52 UTC
Created attachment 131741 [details] [review]
Bug 20817: Update module to use YAML::XS

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 95 Kyle M Hall 2022-03-15 15:50:57 UTC
Created attachment 131742 [details] [review]
Bug 20817: (QA follow-up) Remove size=1 attribute

Resolves QA warning, with reference to bug 28066.
Line 345 of acqui/neworderempty.tt

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 96 Kyle M Hall 2022-03-15 15:51:01 UTC
Created attachment 131743 [details] [review]
Bug 20817: _trim should always return a value

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 97 Kyle M Hall 2022-03-15 15:51:06 UTC
Created attachment 131744 [details] [review]
Bug 20817: (QA follow-up) Fix 'undefined subroutine' error

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 98 Kyle M Hall 2022-03-15 15:51:10 UTC
Created attachment 131745 [details] [review]
Bug 20817: (QA follow-up) Fix _trim

The patch 'trim should always return a value' does not completely
do the job yet. And might have unwanted effects for the '0' case.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 99 Kyle M Hall 2022-03-15 15:51:15 UTC
Created attachment 131746 [details] [review]
Bug 20817: Fix incorrect namespace for subroutine call

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 100 Kyle M Hall 2022-03-15 15:51:20 UTC
Created attachment 131747 [details] [review]
Bug 20817: Import C4::Items::PrepareItemrecordDisplay for neworderempty.pl

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 101 Kyle M Hall 2022-03-15 15:51:25 UTC
Created attachment 131748 [details] [review]
Bug 20817: (QA follow-up) Add new parameter for PrepareItemrecordDisplay
Comment 102 Kyle M Hall 2022-03-15 15:51:29 UTC
Created attachment 131749 [details] [review]
Bug 20817: (QA follow-up) Fix POD for GetMarcFieldsToOrderValues and GetMarcItemFieldsToOrderValues
Comment 103 Kyle M Hall 2022-03-15 15:59:12 UTC
(In reply to Katrin Fischer from comment #84)
> 1) Can you please explain this change in Items.pm? 
> 
> This would also affect the item form in serials IIRC.

Agreed. I've submitted a followup to return the existing behavior for serials.

> 2) GetMarcFieldsToOrderValues in Koha/Acquisition/Utils.pm
> 3) GetMarcItemFieldsToOrderValues in Koha/Acquisition/Utils.pm

It looks like the original author (Devinim) started with one subroutine that handled both, but ended up forking the subroutine without updated the POD. I've submitted a followup to fix the POD for both subroutines.

> 4) Test plan

Done as well!
Comment 104 Katrin Fischer 2022-04-24 10:51:33 UTC
I am sorry, but I need a more detailed test plan. I am not sure what I am looking for here as I have never really used the stage feature much.

Provided test plan:

1) Choose an existing record from "Add order to basket" area either searching a record or clicking "From a staged file" section
2) After you reach "New order" page for existing record you will see new fieldset called "Existing items" which are existing items belong to a record not previously ordered
3) When you choose item(s) from this new tab fields of "Accounting details" tab will be automatically filled
4) When you finished click save button.


What I have tried:

* Exported "Intermediate Perl" from sample database
  http://localhost:8080/cgi-bin/koha/opac-detail.pl?biblionumber=12
* Add to basket > From a new file
* Imported my record
* When I look at the MARC view, I can see the 952

Next I tried:

* Add to basket > From existing record
* Searched for "Intermediate Perl"
* Picked it for my order

Each time, there was no "Existing items" section on the form.

... and now I am stuck.

How do I need to set up the system preferences?
What is the behavior I should see?

My basket was set to create items on order.


Note: We should at least write up a follow-up bug to change the new methods to snake case.
Comment 105 Kyle M Hall 2022-10-25 16:31:36 UTC
Created attachment 142630 [details] [review]
Bug 20817: Add ability to retain 9xx fields when adding on order item to existing record

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

https://bugs.koha-community.org/show_bug.cgi?id=20187
Comment 106 Kyle M Hall 2022-10-25 16:31:54 UTC
Created attachment 142631 [details] [review]
Bug 20817: Move subroutines to a new Perl module
Comment 107 Kyle M Hall 2022-10-25 16:31:59 UTC
Created attachment 142632 [details] [review]
Bug 20817: Add unit tests
Comment 108 Kyle M Hall 2022-10-25 16:32:04 UTC
Created attachment 142633 [details] [review]
Bug 20817: Rename get_infos_syspref_on_item to GetMarcItemFieldsToOrderValues
Comment 109 Kyle M Hall 2022-10-25 16:32:09 UTC
Created attachment 142634 [details] [review]
Bug 20817: Rename get_infos_syspref to GetMarcFieldsToOrderValues
Comment 110 Kyle M Hall 2022-10-25 16:32:18 UTC
Created attachment 142635 [details] [review]
Bug 20817: Remove syspref from parameters, it is invariant
Comment 111 Kyle M Hall 2022-10-25 16:32:25 UTC
Created attachment 142636 [details] [review]
Bug 20817: Fix QA script issues
Comment 112 Kyle M Hall 2022-10-25 16:32:30 UTC
Created attachment 142637 [details] [review]
Bug 20817: Update module to use YAML::XS
Comment 113 Kyle M Hall 2022-10-25 16:32:35 UTC
Created attachment 142638 [details] [review]
Bug 20817: (QA follow-up) Remove size=1 attribute

Resolves QA warning, with reference to bug 28066.
Line 345 of acqui/neworderempty.tt
Comment 114 Kyle M Hall 2022-10-25 16:32:42 UTC
Created attachment 142639 [details] [review]
Bug 20817: _trim should always return a value
Comment 115 Kyle M Hall 2022-10-25 16:32:49 UTC
Created attachment 142640 [details] [review]
Bug 20817: (QA follow-up) Fix 'undefined subroutine' error
Comment 116 Kyle M Hall 2022-10-25 16:32:54 UTC
Created attachment 142641 [details] [review]
Bug 20817: (QA follow-up) Fix _trim

The patch 'trim should always return a value' does not completely
do the job yet. And might have unwanted effects for the '0' case.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 117 Kyle M Hall 2022-10-25 16:32:59 UTC
Created attachment 142642 [details] [review]
Bug 20817: Fix incorrect namespace for subroutine call
Comment 118 Kyle M Hall 2022-10-25 16:33:07 UTC
Created attachment 142643 [details] [review]
Bug 20817: Import C4::Items::PrepareItemrecordDisplay for neworderempty.pl
Comment 119 Kyle M Hall 2022-10-25 16:33:13 UTC
Created attachment 142644 [details] [review]
Bug 20817: (QA follow-up) Add new parameter for PrepareItemrecordDisplay
Comment 120 Kyle M Hall 2022-10-25 16:33:23 UTC
Created attachment 142645 [details] [review]
Bug 20817: (QA follow-up) Fix POD for GetMarcFieldsToOrderValues and GetMarcItemFieldsToOrderValues
Comment 121 Kyle M Hall 2022-10-25 16:41:32 UTC
Created attachment 142646 [details] [review]
Bug 20817: Add ability to retain 9xx fields when adding on order item to existing record

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

https://bugs.koha-community.org/show_bug.cgi?id=20187
Comment 122 Kyle M Hall 2022-10-25 16:41:53 UTC
Created attachment 142647 [details] [review]
Bug 20817: Squashed follow-ups

* Move subroutines to a new Perl module
* Add unit tests
* Rename get_infos_syspref_on_item to GetMarcItemFieldsToOrderValues
* Rename get_infos_syspref to GetMarcFieldsToOrderValues
* Remove syspref from parameters, it is invariant
* Fix QA script issues
* Update module to use YAML::XS
* _trim should always return a value
* (QA follow-up) Fix 'undefined subroutine' error
* Fix incorrect namespace for subroutine call
* Import C4::Items::PrepareItemrecordDisplay for neworderempty.pl
* (QA follow-up) Add new parameter for PrepareItemrecordDisplay
* (QA follow-up) Fix POD for GetMarcFieldsToOrderValues and GetMarcItemFieldsToOrderValues
Comment 123 Kyle M Hall 2022-10-25 16:41:58 UTC
Created attachment 142648 [details] [review]
Bug 20817: (QA follow-up) Remove size=1 attribute

Resolves QA warning, with reference to bug 28066.
Line 345 of acqui/neworderempty.tt
Comment 124 Kyle M Hall 2022-10-25 16:42:04 UTC
Created attachment 142649 [details] [review]
Bug 20817: (QA follow-up) Fix _trim

The patch 'trim should always return a value' does not completely
do the job yet. And might have unwanted effects for the '0' case.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 125 Kyle M Hall 2022-10-25 16:45:18 UTC
Created attachment 142650 [details] [review]
Bug 20817: Add ability to retain 9xx fields when adding on order item to existing record

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 126 Kyle M Hall 2022-10-25 16:48:17 UTC
Created attachment 142651 [details] [review]
Bug 20817: Add ability to retain 9xx fields when adding on order item to existing record

We would like an Acquisitions development that would streamline the workflow of ordering items that already exist in the system (reorders).

The ideal workflow would be something along the lines of:
1. Staged files are added to a basket and a duplicate warning is received. You click "Display them" (This step already exists)
2. From this screen, when you click "add order" on an item, you are able to add that order record to an existing bib record using the information in the 9xx fields. Currently this step discards any 9xx information and creates a blank order record.
3. Click "Save" and move on to the next duplicate.

Since the ability to add an order to an existing record is already present, we're just looking to add the 9xx information as well.

Additional Authors:
Devinim <kohadevinim@devinim.com.tr>
Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 127 Katrin Fischer 2022-10-30 12:15:14 UTC
I know it's hard to keep on top of bugs with many comments, but 2 issues found here:

1) Method names are not snake case (see comment84, comment#104)

2) I still don't know what to do here in order to test this (see comment#104)

The test plan assumes you know a lot of things about the process already. Maybe a test record with 9xx could be supplied?
Comment 128 Katrin Fischer 2022-10-30 12:35:44 UTC
3) POD and use of GetMarcItemFieldsToOrderValues don't match up (first param):

+my $data = GetMarcItemFieldsToOrderValues('MarcItemFieldsToOrder', $marcrecord, ['homebranch', 'holdingbranch', 'itype', 'nonpublic_note', 'public_note', 'loc', 'ccode', 'notforloan', 'uri', 'copyno', 'price', 'replacementprice', 'itemcallnumber', 'quantity', 'budget_code']);

my $alliteminfos = Koha::Acquisition::Utils::GetMarcItemFieldsToOrderValues( $marcrecord, [ 'homebranch', 'holdingbranch', 'itype', 'nonpublic_note', 'public_note', 'loc', 'ccode', 'notforloan', 'uri', 'copyno', 'price', 'replacementprice', 'itemcallnumber', 'quantity', 'budget_code' ] );


4) +use C4::Suggestions;    # GetSuggestion
Should list all used methods explicitly.

Further testing might also be blocked by bug 32045. :(
Comment 129 Nick Clemens 2022-10-31 12:16:19 UTC
Created attachment 142829 [details] [review]
Bug 20817: Add ability to retain 9xx fields when adding on order item to existing record

This patchset intends to add the use of MarcItemFieldsToOrder and MarcFieldsToOrder to
neworderempty.pl

When adding to a basket fomr a staged file the above preferences create orders on addorderiso2709.pl
if the records are unique, or if the records have been matched during staging

If the records are not matched at staging, but a duplicate is found you must add the orders individually, and the info is not pulled from the incoming record

Setup -- Set systempreferences below

MarcFieldsToOrder:
price: 949$g
quantity: 949$k
budget_code: 949$l
discount: 949$m
sort1: 949$n
sort2: 949$q

MarcItemFieldsToOrder:
homebranch: 949$a
holdingbranch: 949$b
itype: 949$y
nonpublic_note: 949$x
public_note: 949$z
loc: 949$c
ccode: 949$8
notforloan: 949$7
uri: 949$u
copyno: 949$t
price: 949$g
replacementprice: 949$v
itemcallnumber: 949$o
quantity: 949$k
budget_code: 949$l

To test:
1 - Stage the sample record for bib303 without looking for matching records (if not using Koha testing docker smaple data import this record, then stage again without matching)
2 - From acqusitions, find a vendor and an open basket and 'Add to basket' 'From a staged file'
3 - On the record in question, check the checbox, then click the 'Add order' link
4 - You are taken to page notifying of duplicate - click 'Use existing record'
5 - You are taken to new order empty, and item forms are prepopulated with order info
6 - Save the orders and confirm all added to basket as expected

Additional Authors:
Devinim <kohadevinim@devinim.com.tr>
Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 130 Katrin Fischer 2022-11-17 11:04:25 UTC
Hi Nick, does this still need a follow-up or back to testing?
Comment 131 Nick Clemens 2022-11-17 11:11:48 UTC
This needs work