Acquisitions: attaching orders to existing records while retaining 9xx information.
Hi Kyle, can you explain this one a bit more?
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.
Thx for the explanation!
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.
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)
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"" 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
Created attachment 86711 [details] [review] Bug 20817: Add ability to retain 9xx fields when adding on order item to existing record
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
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.
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.
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.
(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.
Moving to failed QA, hope this can be fixed!
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
Created attachment 105215 [details] [review] Bug 20817: Move subroutines to a new Perl module
Created attachment 105216 [details] [review] Bug 20817: Add unit tests
Created attachment 105217 [details] [review] Bug 20817: Rename get_infos_syspref_on_item to GetMarcItemFieldsToOrderValues
Created attachment 105218 [details] [review] Bug 20817: Rename get_infos_syspref to GetMarcFieldsToOrderValues
Created attachment 105219 [details] [review] Bug 20817: Remove syspref from parameters, it is invariant
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
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
Created attachment 115343 [details] [review] Bug 20817: Fix QA script issues
Thanks, Kyle! We are really looking forward to getting this fixed.
Willing to do a QA round on this one. Can you please rebase?
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
Created attachment 118372 [details] [review] Bug 20817: Move subroutines to a new Perl module
Created attachment 118373 [details] [review] Bug 20817: Add unit tests
Created attachment 118374 [details] [review] Bug 20817: Rename get_infos_syspref_on_item to GetMarcItemFieldsToOrderValues
Created attachment 118375 [details] [review] Bug 20817: Rename get_infos_syspref to GetMarcFieldsToOrderValues
Created attachment 118376 [details] [review] Bug 20817: Remove syspref from parameters, it is invariant
Created attachment 118377 [details] [review] Bug 20817: Fix QA script issues
Created attachment 118378 [details] [review] Bug 20817: Update module to use YAML::XS
Looking here
FAIL acqui/neworderempty.pl FAIL valid Useless use of a variable in void context
(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..
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)
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
(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 ;)
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
(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 } ;)
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.
Created attachment 122834 [details] [review] Bug 20817: _trim should always return a value
my $string = shift; return unless $string; $string =~ s/^\s+|\s+$//g; - return $string; + return $string // q{}; Note the first return too.
(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
Created attachment 122841 [details] [review] Bug 20817: (QA follow-up) Fix 'undefined subroutine' error
Having another look here
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.
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>
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>
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.
"Util" sounds wrong, it's a too generic term IMO.
(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!
(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.
Hi, get_infos_syspref() should be using C4::Context->yaml_preference instead of manually dealing with YAML.
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
Created attachment 128767 [details] [review] Bug 20817: Move subroutines to a new Perl module
Created attachment 128768 [details] [review] Bug 20817: Add unit tests
Created attachment 128769 [details] [review] Bug 20817: Rename get_infos_syspref_on_item to GetMarcItemFieldsToOrderValues
Created attachment 128770 [details] [review] Bug 20817: Rename get_infos_syspref to GetMarcFieldsToOrderValues
Created attachment 128771 [details] [review] Bug 20817: Remove syspref from parameters, it is invariant
Created attachment 128772 [details] [review] Bug 20817: Fix QA script issues
Created attachment 128773 [details] [review] Bug 20817: Update module to use YAML::XS
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
Created attachment 128775 [details] [review] Bug 20817: _trim should always return a value
Created attachment 128776 [details] [review] Bug 20817: (QA follow-up) Fix 'undefined subroutine' error
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>
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
Created attachment 128782 [details] [review] Bug 20817: Fix incorrect namespace for subroutine call
Created attachment 128783 [details] [review] Bug 20817: Import C4::Items::PrepareItemrecordDisplay for neworderempty.pl
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>
Created attachment 128786 [details] [review] Bug 20817: Move subroutines to a new Perl module Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 128787 [details] [review] Bug 20817: Add unit tests Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 128788 [details] [review] Bug 20817: Rename get_infos_syspref_on_item to GetMarcItemFieldsToOrderValues Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 128789 [details] [review] Bug 20817: Rename get_infos_syspref to GetMarcFieldsToOrderValues Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 128790 [details] [review] Bug 20817: Remove syspref from parameters, it is invariant Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 128791 [details] [review] Bug 20817: Fix QA script issues Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 128792 [details] [review] Bug 20817: Update module to use YAML::XS Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
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>
Created attachment 128794 [details] [review] Bug 20817: _trim should always return a value Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 128795 [details] [review] Bug 20817: (QA follow-up) Fix 'undefined subroutine' error Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
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>
Created attachment 128797 [details] [review] Bug 20817: Fix incorrect namespace for subroutine call Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 128798 [details] [review] Bug 20817: Import C4::Items::PrepareItemrecordDisplay for neworderempty.pl Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
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.
Created attachment 131732 [details] [review] Bug 20817: (QA follow-up) Add new parameter for PrepareItemrecordDisplay
Created attachment 131733 [details] [review] Bug 20817: (QA follow-up) Fix POD for GetMarcFieldsToOrderValues and GetMarcItemFieldsToOrderValues
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>
Created attachment 131735 [details] [review] Bug 20817: Move subroutines to a new Perl module Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 131736 [details] [review] Bug 20817: Add unit tests Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 131737 [details] [review] Bug 20817: Rename get_infos_syspref_on_item to GetMarcItemFieldsToOrderValues Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 131738 [details] [review] Bug 20817: Rename get_infos_syspref to GetMarcFieldsToOrderValues Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 131739 [details] [review] Bug 20817: Remove syspref from parameters, it is invariant Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 131740 [details] [review] Bug 20817: Fix QA script issues Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 131741 [details] [review] Bug 20817: Update module to use YAML::XS Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
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>
Created attachment 131743 [details] [review] Bug 20817: _trim should always return a value Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 131744 [details] [review] Bug 20817: (QA follow-up) Fix 'undefined subroutine' error Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
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>
Created attachment 131746 [details] [review] Bug 20817: Fix incorrect namespace for subroutine call Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 131747 [details] [review] Bug 20817: Import C4::Items::PrepareItemrecordDisplay for neworderempty.pl Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 131748 [details] [review] Bug 20817: (QA follow-up) Add new parameter for PrepareItemrecordDisplay
Created attachment 131749 [details] [review] Bug 20817: (QA follow-up) Fix POD for GetMarcFieldsToOrderValues and GetMarcItemFieldsToOrderValues
(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!
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.
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
Created attachment 142631 [details] [review] Bug 20817: Move subroutines to a new Perl module
Created attachment 142632 [details] [review] Bug 20817: Add unit tests
Created attachment 142633 [details] [review] Bug 20817: Rename get_infos_syspref_on_item to GetMarcItemFieldsToOrderValues
Created attachment 142634 [details] [review] Bug 20817: Rename get_infos_syspref to GetMarcFieldsToOrderValues
Created attachment 142635 [details] [review] Bug 20817: Remove syspref from parameters, it is invariant
Created attachment 142636 [details] [review] Bug 20817: Fix QA script issues
Created attachment 142637 [details] [review] Bug 20817: Update module to use YAML::XS
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
Created attachment 142639 [details] [review] Bug 20817: _trim should always return a value
Created attachment 142640 [details] [review] Bug 20817: (QA follow-up) Fix 'undefined subroutine' error
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>
Created attachment 142642 [details] [review] Bug 20817: Fix incorrect namespace for subroutine call
Created attachment 142643 [details] [review] Bug 20817: Import C4::Items::PrepareItemrecordDisplay for neworderempty.pl
Created attachment 142644 [details] [review] Bug 20817: (QA follow-up) Add new parameter for PrepareItemrecordDisplay
Created attachment 142645 [details] [review] Bug 20817: (QA follow-up) Fix POD for GetMarcFieldsToOrderValues and GetMarcItemFieldsToOrderValues
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
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
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
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>
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>
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>
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?
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. :(
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>
Hi Nick, does this still need a follow-up or back to testing?
This needs work