Bug 10877 - Add "Order Record" processing
Summary: Add "Order Record" processing
Status: CLOSED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Keywords:
: 9171 (view as bug list)
Depends on: 7180
Blocks:
  Show dependency treegraph
 
Reported: 2013-09-11 19:35 UTC by Kyle M Hall
Modified: 2016-12-05 21:23 UTC (History)
13 users (show)

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


Attachments
Bug 10877 - Add "Order Record" processing (19.96 KB, patch)
2013-09-11 19:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10877 - Add "Order Record" processing (19.42 KB, patch)
2013-09-12 11:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10877 - Add "Order Record" processing (20.72 KB, patch)
2013-09-24 18:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10877 - Add "Order Record" processing (17.68 KB, patch)
2014-01-16 16:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10877 - Add "Order Record" processing (20.09 KB, patch)
2014-01-16 16:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10877 - Add "Order Record" processing (20.81 KB, patch)
2014-01-16 17:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10877 - Add "Order Record" processing (21.30 KB, patch)
2014-07-01 12:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10877 - Add "Order Record" processing (21.22 KB, patch)
2014-07-01 12:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10877 - Add "Order Record" processing (20.16 KB, patch)
2014-07-16 15:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10877 - Add "Order Record" processing (20.22 KB, patch)
2014-07-16 15:35 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10877 - Add "Order Record" processing (20.26 KB, patch)
2014-09-03 12:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10877 - Add "Order Record" processing (20.19 KB, patch)
2015-02-05 18:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10877 - Add "Order Record" processing (19.92 KB, patch)
2015-05-05 13:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10877 [QA Followup] (1.62 KB, patch)
2015-05-05 13:19 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 2013-09-11 19:35:51 UTC
Order Record Processing will allow a library to stage an "order record" file which is a standard marc file with some additional information in it about how to create items automatically ( quantity, itemtype, etc ). The location of these fields is defined in the system preference MarcFieldsToOrder from bug 7180.

The workflow is thus:
1) A librarian uploads an "order record" file, and marks the batch as an order file during the staging process.
2) The librarian selects an acquisitions basket and chooses "from a staged order file"
3) From here, the librarian can view all the records that will be created, along with quantity and other data ( from bug 7180 ). The librarian will *not* see the item fields, as those are automatically created using the minimum data needed ( branches, itype ).
4) The librarian hits "save" and the items are automatically generated on order.

Later ( using features not directly tied to this feature ), the librarian will receive a new marc batch file with items attached ( including itemnumbers ). The vendor will have this information because it was sent via EDI ( bug 7736 ). The librarian will then use the marc record staging feature to overlay those bare bones item records with the full data ( via bug 7130 ).
Comment 1 Kyle M Hall 2013-09-11 19:36:48 UTC
Bug 7130 should be 7131.
Comment 2 Kyle M Hall 2013-09-11 19:39:05 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2013-09-12 11:39:15 UTC Comment hidden (obsolete)
Comment 4 I'm just a bot 2013-09-18 11:12:23 UTC
Applying: Bug 10877 - Add "Order Record" processing
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 10877 - Add "Order Record" processing
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Comment 5 Jonathan Druart 2013-09-18 12:04:19 UTC
Depend on 7180.
Comment 6 Kyle M Hall 2013-09-24 18:41:26 UTC Comment hidden (obsolete)
Comment 7 I'm just a bot 2013-09-29 06:15:28 UTC
Patch applied cleanly, go forth and signoff
Comment 8 I'm just a bot 2013-10-27 06:29:18 UTC
Applying: Bug 7180: Add UT for C4::Budgets::GetBudgetByCode
Using index info to reconstruct a base tree...
M	t/db_dependent/Budgets.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Budgets.t
CONFLICT (content): Merge conflict in t/db_dependent/Budgets.t
Patch failed at 0001 Bug 7180: Add UT for C4::Budgets::GetBudgetByCode
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Comment 9 I'm just a bot 2013-10-27 06:29:32 UTC
This bug depends on bug7180 which is in status Needs Signoff but the patches for it do not apply cleanly
Comment 10 Kyle M Hall 2014-01-16 16:51:23 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2014-01-16 16:56:31 UTC Comment hidden (obsolete)
Comment 12 Kyle M Hall 2014-01-16 17:20:27 UTC Comment hidden (obsolete)
Comment 13 Paola Rossi 2014-05-21 14:37:28 UTC
I test against master 3.15.00.052

Applying: Bug 10877 - Add "Order Record" processing
fatal: sha1 information is lacking or useless (acqui/addorderiso2709.pl).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 10877 - Add "Order Record" processing

So I pass the patch to "Patch doesn't apply" status
Comment 14 Kyle M Hall 2014-07-01 12:26:26 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2014-07-01 12:32:11 UTC Comment hidden (obsolete)
Comment 16 Aaron Sakovich 2014-07-08 20:38:57 UTC
We've tested this extensively and have seen no problems with the upload and automatic order generation.
Comment 17 Jonathan Druart 2014-07-09 14:30:27 UTC
Comment on attachment 29406 [details] [review]
Bug 10877 - Add "Order Record" processing

Review of attachment 29406 [details] [review]:
-----------------------------------------------------------------

::: C4/ImportBatch.pm
@@ +967,3 @@
>  =cut
>  
>  sub GetImportBatchRangeDesc {

You should provide unit test here, but it seems to be quite difficult here (no test exists for this module).

::: acqui/addorderiso2709.pl
@@ +270,5 @@
>              my @serials      = $input->param('serial');
>              my @ind_tag   = $input->param('ind_tag');
>              my @indicator = $input->param('indicator');
> +
> +            if ($is_order) {

Could you detail why you process these 4 fields?

@@ +295,5 @@
> +                  GetMarcFromKohaField('items.itype');
> +                push( @tags,         $itype_field );
> +                push( @subfields,    $itype_subfield );
> +                push( @field_values, $infos->{itype} );
> +                warn "ITYPE: " . $infos->{itype};

useless warn

@@ +359,1 @@
>  

hum, good candidate for an hashref param.

@@ +450,5 @@
>                          item_action => $item_action
>                      );
>      batch_info($template, $batch);
> +
> +    return \@list;

Why did you change this return?

::: installer/data/mysql/updatedatabase.pl
@@ +8565,5 @@
> +    $dbh->do("ALTER TABLE import_batches ADD is_order BOOLEAN NOT NULL DEFAULT '0' AFTER comments");
> +   print "Upgrade to $DBversion done (Bug 10877 - Add 'Order Record' processing)\n";
> +   SetVersion ($DBversion);
> +
> +}

FAIL   pod
                *** WARNING: =head2 without preceding higher level  in file installer/data/mysql/updatedatabase.pl

Just add an blank line before =head1

::: koha-tmpl/intranet-tmpl/prog/en/includes/acquisitions-add-to-basket.inc
@@ +18,4 @@
>          <li><a href="/cgi-bin/koha/acqui/z3950_search.pl?booksellerid=[% booksellerid %]&amp;basketno=[% basketno %]">From an external source</a></li>
>          <li><a href="/cgi-bin/koha/acqui/addorderiso2709.pl?booksellerid=[% booksellerid %]&amp;basketno=[% basketno %]"> From a staged file</a></li>
>          [% IF ( CAN_user_circulate ) %]<li><a href="/cgi-bin/koha/circ/reserveratios.pl?booksellerid=[% booksellerid %]&amp;basketno=[% basketno %]">From titles with highest hold ratios</a></li>[% END %]
> +        <li><a href="/cgi-bin/koha/acqui/addorderiso2709.pl?booksellerid=[% booksellerid %]&amp;basketno=[% basketno %]&amp;is_order=1"> From a staged order file</a></li>

Maybe should be put just after "from a staged file"?

::: koha-tmpl/intranet-tmpl/prog/en/modules/acqui/addorderiso2709.tt
@@ +145,5 @@
> +            $("#checkAll").click();
> +
> +            $("#all_budget_id").hide();
> +            $('label[for="all_budget_id"]').hide();
> +        [% END %]

I don't understand the trick here.
Please check because the behavior is buggy (try to check/uncheck "Show all fund", the fund list will become empty).
Comment 18 Kyle M Hall 2014-07-16 15:33:20 UTC
Comment on attachment 29406 [details] [review]
Bug 10877 - Add "Order Record" processing

Review of attachment 29406 [details] [review]:
-----------------------------------------------------------------

::: C4/ImportBatch.pm
@@ +967,3 @@
>  =cut
>  
>  sub GetImportBatchRangeDesc {

Agreed. There are no unit tests for this. I think it would be extremely difficult to unit test. However, the additions I've made to it are close to trivial.

::: acqui/addorderiso2709.pl
@@ +270,5 @@
>              my @serials      = $input->param('serial');
>              my @ind_tag   = $input->param('ind_tag');
>              my @indicator = $input->param('indicator');
> +
> +            if ($is_order) {

This is so we can pre-set the items to a notforloan value of 'ordered', and to preset the home and holding fields to the current branch, as well as pre-set the itemtype.

@@ +295,5 @@
> +                  GetMarcFromKohaField('items.itype');
> +                push( @tags,         $itype_field );
> +                push( @subfields,    $itype_subfield );
> +                push( @field_values, $infos->{itype} );
> +                warn "ITYPE: " . $infos->{itype};

Will fix!

@@ +359,1 @@
>  

While I agree, I wanted to make as few changes as possible to that subroutine.

@@ +450,5 @@
>                          item_action => $item_action
>                      );
>      batch_info($template, $batch);
> +
> +    return \@list;

I just thought it sensible to have the sub return something. Considering no caller of the sub takes in the return value, I expect it will be compiled away. I can remove this if you think it prudent.

::: installer/data/mysql/updatedatabase.pl
@@ +8565,5 @@
> +    $dbh->do("ALTER TABLE import_batches ADD is_order BOOLEAN NOT NULL DEFAULT '0' AFTER comments");
> +   print "Upgrade to $DBversion done (Bug 10877 - Add 'Order Record' processing)\n";
> +   SetVersion ($DBversion);
> +
> +}

Will fix!

::: koha-tmpl/intranet-tmpl/prog/en/includes/acquisitions-add-to-basket.inc
@@ +18,4 @@
>          <li><a href="/cgi-bin/koha/acqui/z3950_search.pl?booksellerid=[% booksellerid %]&amp;basketno=[% basketno %]">From an external source</a></li>
>          <li><a href="/cgi-bin/koha/acqui/addorderiso2709.pl?booksellerid=[% booksellerid %]&amp;basketno=[% basketno %]"> From a staged file</a></li>
>          [% IF ( CAN_user_circulate ) %]<li><a href="/cgi-bin/koha/circ/reserveratios.pl?booksellerid=[% booksellerid %]&amp;basketno=[% basketno %]">From titles with highest hold ratios</a></li>[% END %]
> +        <li><a href="/cgi-bin/koha/acqui/addorderiso2709.pl?booksellerid=[% booksellerid %]&amp;basketno=[% basketno %]&amp;is_order=1"> From a staged order file</a></li>

Will fix!

::: koha-tmpl/intranet-tmpl/prog/en/modules/acqui/addorderiso2709.tt
@@ +145,5 @@
> +            $("#checkAll").click();
> +
> +            $("#all_budget_id").hide();
> +            $('label[for="all_budget_id"]').hide();
> +        [% END %]

I believe this is causing issues do to the fact that this patch targeted an earlier version of bug 7180. I will update the patch.
Comment 19 Kyle M Hall 2014-07-16 15:34:01 UTC Comment hidden (obsolete)
Comment 20 Kyle M Hall 2014-07-16 15:35:18 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2014-07-17 10:54:57 UTC
(In reply to Kyle M Hall from comment #18)
> Comment on attachment 29406 [details] [review] [review]
> ::: acqui/addorderiso2709.pl
> @@ +270,5 @@
> >              my @serials      = $input->param('serial');
> >              my @ind_tag   = $input->param('ind_tag');
> >              my @indicator = $input->param('indicator');
> > +
> > +            if ($is_order) {
> 
> This is so we can pre-set the items to a notforloan value of 'ordered', and
> to preset the home and holding fields to the current branch, as well as
> pre-set the itemtype.

Yes but you hard-code arbitrarily these values.
Why these 4 values and not others?
I would like to get another QA point of view here.
Katrin maybe?
Comment 22 Kyle M Hall 2014-07-17 13:44:52 UTC
-1 is the standard value for 'ordered' in Koha. This feature is to process order records so the items are by definition ordered and not any other status. Additionally we grab the itemtype from the system preference, and we set the branch selects to the logged in branch. I wouldn't say it's arbitrary. If you have any suggestions on how to better handle this, I'd be open to making changes. As the author of the feature this builds on, I think you are probably in the best position to qa this patch.

Kyle

(In reply to Jonathan Druart from comment #21)
> (In reply to Kyle M Hall from comment #18)
> > Comment on attachment 29406 [details] [review] [review] [review]
> > ::: acqui/addorderiso2709.pl
> > @@ +270,5 @@
> > >              my @serials      = $input->param('serial');
> > >              my @ind_tag   = $input->param('ind_tag');
> > >              my @indicator = $input->param('indicator');
> > > +
> > > +            if ($is_order) {
> > 
> > This is so we can pre-set the items to a notforloan value of 'ordered', and
> > to preset the home and holding fields to the current branch, as well as
> > pre-set the itemtype.
> 
> Yes but you hard-code arbitrarily these values.
> Why these 4 values and not others?
> I would like to get another QA point of view here.
> Katrin maybe?
Comment 23 Marcel de Rooy 2014-09-03 12:28:31 UTC
Will have a look at this one, unless another QAer is faster :)
Comment 24 Marcel de Rooy 2014-09-03 12:30:53 UTC Comment hidden (obsolete)
Comment 25 Marcel de Rooy 2014-09-03 12:31:10 UTC
Trivial rebase.
Comment 26 Marcel de Rooy 2014-09-03 13:50:30 UTC
QA Comment:
Looks quite good to me. Some small details still need attention (see also earlier remark of Jonathan):

my $infos = get_infos_syspref($marcrecord, ['itype']);
This does not seem to make sense. According to the description of the pref itype is not contained in the pref. (So you create/use an undocumented feature here?) Note that if I just comment those lines, the items will fall back to the item type that I had in 942c of the biblio record somehow? Any idea where that comes from? 
On the other hand, we could add this to the syspref description and keep it.

Although I do not mind to set the home and holding branch of the item as you do now in the same code region, it is not really necessary. They could be null as well. If I comment the lines, the items will be created too. (Note that I do not specifically understand the concerns of Jonathan for this field.)

Your argument on the notforloan to -1 makes sense to me. I would suggest to keep that one at least.

Changing status to reflect need for small adjustments.
Comment 27 Katrin Fischer 2014-09-14 22:39:12 UTC
Hi Kyle and Marcel,
I know I am late to this, but what is the difference between the 2 options 'order from a staged file' and 'order from a staged order file'? Why do those need to be kept separate?
Comment 28 Kyle M Hall 2014-09-19 17:36:36 UTC
Katrin, they need to be kept separate because order files behave differently than traditional marc files.

(In reply to Katrin Fischer from comment #27)
> Hi Kyle and Marcel,
> I know I am late to this, but what is the difference between the 2 options
> 'order from a staged file' and 'order from a staged order file'? Why do
> those need to be kept separate?
Comment 29 Katrin Fischer 2014-09-21 13:16:20 UTC
So now I know as much as before ;) Can you please explain the direction this goes? What makes them behave differently and how?
Comment 30 Kyle M Hall 2014-10-20 12:56:25 UTC
(In reply to Katrin Fischer from comment #29)
> So now I know as much as before ;) Can you please explain the direction this
> goes? What makes them behave differently and how?

What you stage a record file, you have the option of saying "this is an order record". This is how Koha knows if file is an order file and not a standard marc record file.

The difference is how an order file is set up. In a traditional marc record, you have items in 952, one for each item. In an order file, you have simply have a field for quantity, a field for budget code, etc. These are defined in MarcFieldsToOrder. This pref can also be used for regular marc files. The difference is that order records have not for loan, home and holding branch, and itemtype preset as well.

So basically, order records have more data preset and are less "editable" compared to regular marc records.

The workflow goes something like this:
1) You place an order with your vendor from their website
2) You download the order record and stage in in Koha
3) You create a basket and import the order records, which automatically have the data pre-set to match the order you placed on the vendor's website.
Comment 31 Katrin Fischer 2014-10-22 11:26:55 UTC
Hm, if the only difference is the prepopulating of the item fields (which we shoudl document :) ), then I'd suggest to not use a different order link, but maybe use the existing 'from staged file' and add a column to the list of files that clearly indicates, that the file was marked as 'order record'. Then from there it would just go the normal way, with the difference of prepopulating the fields.

I am not sure if there are more features planned for this, but maybe we need to think about some kind of import template on vendor level at some point - so we don't need to hardcode things, but can have some more flexibility and granularity.
Comment 32 Katrin Fischer 2014-11-02 09:50:22 UTC
*** Bug 9171 has been marked as a duplicate of this bug. ***
Comment 33 Katrin Fischer 2014-12-27 18:39:30 UTC
I feel like I don't understand this feature well - at least not the differences to the existing staged MARC records workflow.

Marcel noted some concerns in comment #26 and earlier Jonathan worked some on this - comment #17.

Can we try to restart the discussion together?
Comment 34 Kyle M Hall 2015-02-05 18:09:38 UTC Comment hidden (obsolete)
Comment 35 Kyle M Hall 2015-02-05 18:11:40 UTC
(In reply to Katrin Fischer from comment #33)
> I feel like I don't understand this feature well - at least not the
> differences to the existing staged MARC records workflow.

I'll see what I can do to make this happen!
Comment 36 Kyle M Hall 2015-05-05 13:13:12 UTC
Created attachment 38849 [details] [review]
Bug 10877 - Add "Order Record" processing

Order Record Processing will allow a library to stage an "order record"
file which is a standard marc file with some additional information in
it about how to create items automatically ( quantity, itemtype, etc ).
The location of these fields is defined in the system preference
MarcFieldsToOrder from bug 7180.

The workflow is thus:
1) A librarian uploads an "order record" file, and marks the batch as an
order file during the staging process.
2) The librarian selects an acquisitions basket and chooses "from a
staged order file"
3) From here, the librarian can view all the records that will be
created, along with quantity and other data ( from bug 7180 ). The
librarian will *not* see the item fields, as those are automatically
created using the minimum data needed ( branches, itype ).
4) The librarian hits "save" and the items are automatically generated
on order.

Later ( using features not directly tied to this feature ), the
librarian will receive a new marc batch file with items attached
( including itemnumbers ). The vendor will have this information
because it was sent via EDI ( bug 7736 ). The librarian will then use
the marc record staging feature to overlay those bare bones item records
with the full data ( via bug 7131 ).

Signed-off-by: Aaron Sakovich <asakovich@hmcpl.org>
Comment 37 Kyle M Hall 2015-05-05 13:19:06 UTC
Created attachment 38850 [details] [review]
Bug 10877 [QA Followup]
Comment 38 Jonathan Druart 2015-06-19 11:55:58 UTC
(In reply to Kyle M Hall from comment #35)
> (In reply to Katrin Fischer from comment #33)
> > I feel like I don't understand this feature well - at least not the
> > differences to the existing staged MARC records workflow.
> 
> I'll see what I can do to make this happen!

Did you do something since this comment?
Is it related to the second patch [QA Followup]?
Comment 39 Jonathan Druart 2015-06-23 09:42:45 UTC
Waiting for an answer.
Comment 40 Kyle M Hall 2015-07-30 10:15:47 UTC
Order records, at an item level, work much differently than traditional marc records. A standard marc record for Koha would have a 952 field for each item with the appropriate subfields.

An order record, on the other hand, has *no* 952 field. Instead it has data that tells us how to create the items ourselves, with fields/subfields for the itemtype, collectioncode, acquisitions fund, etc defined in the record in  a non 952-field ( it seems that 947 is often used for this ). Most importantly, that is a field that defines the quantity of items to create.



(In reply to Jonathan Druart from comment #38)
> (In reply to Kyle M Hall from comment #35)
> > (In reply to Katrin Fischer from comment #33)
> > > I feel like I don't understand this feature well - at least not the
> > > differences to the existing staged MARC records workflow.
> > 
> > I'll see what I can do to make this happen!
> 
> Did you do something since this comment?
> Is it related to the second patch [QA Followup]?
Comment 41 Kyle M Hall 2015-07-30 10:30:42 UTC
I forgot to mention the items on an order record are automatically marked as being not for loan ( since they are on order ) and have home and holding branch automatically set to the logged in library.
Comment 42 Jonathan Druart 2015-09-23 12:17:38 UTC
Kyle,

Sorry about that but I am switching the status back to Needs Signoff.
The idea is not to try and block this enhancement but, on the contrary, to get feedbacks from someone else.
I have CCed some people which might be interested in this feature.

Hope to get it back in the QA queue soon!
Comment 43 I'm just a bot 2016-03-06 21:47:14 UTC
fatal: previous rebase directory .git/rebase-apply still exists but mbox given. at ./getter.pl line 196.
Comment 44 Kyle M Hall 2016-03-07 15:20:27 UTC
I believe we are pursuing an alternative solution and will continue to get this patch into Koha.