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: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Devinim
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-05-24 17:16 UTC by Kyle M Hall
Modified: 2021-03-17 13:46 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
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

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