Bug 11844 - Additional fields for order lines
Summary: Additional fields for order lines
Status: BLOCKED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature (vote)
Assignee: Julian Maurice
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 15774 10855 11944 13596
Blocks: 11870
  Show dependency treegraph
 
Reported: 2014-02-26 09:42 UTC by Julian Maurice
Modified: 2017-10-23 17:33 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 11844: Use additional fields for order lines (19.01 KB, patch)
2014-02-26 09:43 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11844: Use additional fields for order lines (19.07 KB, patch)
2014-02-26 11:10 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11844: Copy additional fields values when transfering order (1.25 KB, patch)
2015-08-24 13:32 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11844: Fix additional field values when creating a new order line (908 bytes, patch)
2015-08-24 13:32 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11844: Use additional fields for order lines (20.07 KB, patch)
2015-12-14 17:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11844: Copy additional fields values when transfering order (1.25 KB, patch)
2015-12-14 17:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11844: Fix additional field values when creating a new order line (914 bytes, patch)
2015-12-14 17:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
picture of warning for AV and Marcfield (138.80 KB, image/png)
2015-12-17 10:57 UTC, jmbroust
Details
Bug 11844: Use additional fields for order lines (20.13 KB, patch)
2015-12-22 17:44 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 11844: Copy additional fields values when transfering order (1.31 KB, patch)
2015-12-22 17:44 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 11844: Fix additional field values when creating a new order line (969 bytes, patch)
2015-12-22 17:45 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 11844: Use additional fields for order lines (19.62 KB, patch)
2016-07-21 20:49 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 11844: Copy additional fields values when transfering order (1.24 KB, patch)
2016-07-21 20:50 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 11844: Fix additional field values when creating a new order line (989 bytes, patch)
2016-07-21 20:50 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 11844: (followup) use new unified additional field screen (12.94 KB, patch)
2016-07-21 20:50 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 11844: (followup) use new unified additional field screen (15.20 KB, patch)
2016-07-21 21:55 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 11844: Fix syntax errors (1.03 KB, patch)
2016-09-13 13:33 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2014-02-26 09:42:26 UTC
Make use of new module Koha::AdditionalField to provide additional fields to order lines.
Comment 1 Julian Maurice 2014-02-26 09:43:10 UTC Comment hidden (obsolete)
Comment 2 Julian Maurice 2014-02-26 11:10:07 UTC Comment hidden (obsolete)
Comment 3 Jacek Ablewicz 2014-03-04 09:04:46 UTC
Bug 10855 as a whole does not apply at the moment (may need a little rebase ?), so I tried to test this patch with Koha/AdditionalField.pm alone extracted from 10855. Found some problems:
1) if order line is deleted, additional fields for this order will stay in the database
2) in case order gets transferred to another basket, additional fields will remain associated with initial order record (now "cancelled" after the transfer); shouldn't they rather got re-associated with the newly created order record (the clone of the original) instead?
3) non-latin character encoding: similar issues as in 10855 (see also Bug 11302 - Template::output should deal with object).

Regarding 3), I have working solution for bug 10855 (essentially, it's a more conservative take than Jonathan patch in 11302 - should be less regression-prone; still needs some testing), but it doesn't quite work for this particular patch. I guess it's because new sub GetValues() doesn't encode it's output, which introduces yet another (different kind of) encoding problem in cases where there are non-latin characters present e.g. in authorized values descriptions (?).
Comment 4 Yves Tomic 2014-03-13 09:12:42 UTC
I've tested this patch and it is working. You can add easily new fields linked to a order line. This function could be very useful to add specific fields to manage your collections.

It is true that if a order line is being deleted, additional fields for this order will be stored further in the database, precisely in the table "additional_field_values" but it's a non-blocking feature. Of course, it would be better to have these additional fields deleted too when the order line is being deleted.

The encoding problem should be solved. I have created additional fields in french with accented letters and the display is not ok.

If the two last issues are fixed, I think that this patch will improve the acquisitions module.
Comment 5 Julian Maurice 2015-02-12 12:39:26 UTC
Changing status to Failed QA, as it requires some work
Comment 6 Julian Maurice 2015-08-24 13:28:49 UTC
(In reply to Jacek Ablewicz from comment #3)
> 1) if order line is deleted, additional fields for this order will
> stay in the database

Order lines are never removed from database, so additional fields data should remain too.

> 3) non-latin character encoding: similar issues as in 10855 (see also
> Bug 11302 - Template::output should deal with object).

I think this has been fixed by bug 11944 or bug 10855 because I can't reproduce the problem.


Remaining issue:
> 2) in case order gets transferred to another basket, additional fields
> will remain associated with initial order record (now "cancelled"
> after the transfer); shouldn't they rather got re-associated with the
> newly created order record (the clone of the original) instead?
Comment 7 Julian Maurice 2015-08-24 13:32:48 UTC Comment hidden (obsolete)
Comment 8 Julian Maurice 2015-08-24 13:32:52 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2015-12-14 17:11:11 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2015-12-14 17:11:18 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2015-12-14 17:11:21 UTC Comment hidden (obsolete)
Comment 12 jmbroust 2015-12-17 10:57:25 UTC
Created attachment 45758 [details]
picture of warning for AV and Marcfield

When I try to set up parameters for test case :

d/ a AV category, a MARC field not linked to AV category

then I have a blocking warning (see picture attached).
Am I doing wrong ?
Comment 13 sandboxes@biblibre.com 2015-12-22 17:44:15 UTC
Patch tested with a sandbox, by Harold Dramer <harold.dramer@nyls.edu>
Comment 14 sandboxes@biblibre.com 2015-12-22 17:44:54 UTC Comment hidden (obsolete)
Comment 15 sandboxes@biblibre.com 2015-12-22 17:44:57 UTC Comment hidden (obsolete)
Comment 16 sandboxes@biblibre.com 2015-12-22 17:45:00 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2016-01-07 11:09:02 UTC
Since I am the author of bug 10855, it would be better to see someone else QA this new enhancement.
Comment 18 Jonathan Druart 2016-03-22 10:29:45 UTC
Jesse, any chances to see you QA this one?
Comment 19 Katrin Fischer 2016-03-28 21:50:39 UTC
Just some first notes on this:

1) New "add order line fields" page

a) Test plan calls for adding fields with AV category and MARC field, but this always results in this error message for me:
You cannot select an authorised value category and a marcfield

b) why have the datatable search field below the table instead of above? I know we have other places where it's like this, but it always irritates me - the search is most useful for a big table and this will require you to scroll all the way to the end of the table first.

c) this looks like a tiny translation issue:
'aLengthMenu': [[10, 20, 50, 100, -1], [10, 20, 50, 100, "All"]],

d) it would be nice, if the table could use the new buttons we for Edit and Delete as introduced for a lot of other tables recently to match the new design pattern.

e) Icon icon-plus and others should be FA icons.

Otherwise this seems to work ok. :)

I am wondering what the next step is - this patch will allow to see and manipulate the fields on adding an order, but later, once the basket is closed you won't be able to see or search for them. So we should try and get bug 11870 sorted and maybe think about a way to show the fields on receive and on the basket summary page (in separate bugs).
Comment 20 Jonathan Druart 2016-04-04 12:21:29 UTC
(In reply to Katrin Fischer from comment #19)
> Just some first notes on this:
> 
> 1) New "add order line fields" page
> 
> a) Test plan calls for adding fields with AV category and MARC field, but
> this always results in this error message for me:
> You cannot select an authorised value category and a marcfield

Yes, the test plan is wrong: you cannot define both.

> b) why have the datatable search field below the table instead of above? I
> know we have other places where it's like this, but it always irritates me -
> the search is most useful for a big table and this will require you to
> scroll all the way to the end of the table first.

It's the same behaviour as for serials, so I'd say it's expected.

> d) it would be nice, if the table could use the new buttons we for Edit and
> Delete as introduced for a lot of other tables recently to match the new
> design pattern.

Could be done by bug 15774 which create a new page to configure all of these additional fields.
Comment 21 Katrin Fischer 2016-04-04 12:26:30 UTC
>> b) why have the datatable search field below the table instead of above? I
>> know we have other places where it's like this, but it always irritates me -
>> the search is most useful for a big table and this will require you to
>> scroll all the way to the end of the table first.

>It's the same behaviour as for serials, so I'd say it's expected.

Still doesn't make sense :) But not a blocker. 
I think Jesse wanted to take a look at this - so I didn't test further.
Comment 22 Jesse Weaver 2016-07-21 20:49:43 UTC
Created attachment 53614 [details] [review]
Bug 11844: Use additional fields for order lines

This patch makes use of new module Koha::AdditionalField to provide
additional fields to order lines.
Once created, these fields can be filled during order line creation or
modification.

If additional field is linked to a MARC field, then
value from biblio record is retrieved at order line creation. When
saving order line (at creation or modification), values in additional
fields are saved into biblio record.
If additional field is linked to an authorised value category, then
authorised values are used. If not directly linked to an authorised
value category, but linked to a MARC field, a search for a AV category
is made on MARC default framework.

This patch doesn't display additional fields value anywhere (except in
order line creation/modification). Future patches will do that.

Test plan:
1/ Go to Acquisitions home
2/ In the left menu, click on "Add order line fields"
3/ Click on "New field" button
4/ Give the field a name (unique), no AV category and no MARC field.
5/ Save.
6/ Create 5 other fields:
   a/ no AV category, a MARC field not linked to AV category
   b/ no AV category, a MARC field linked to AV category
   c/ a AV category, no MARC field
   d/ a AV category, a MARC field not linked to AV category
   e/ a AV category, a MARC field linked to another AV category
   In case of (e), the additional AV category should be used, not the AV
   category linked to MARC field
7/ Create everything you need to be able to create order lines
   (supplier, basket, ...)
8/ Create an order line. At bottom of the page, you should see your
   additional fields, with authorised values dropdrown list for fields
   (c), (d) and (e) (check this is the right AV category for (e))
9/ Fill these fields with some data and save order line
10/ check that data was correctly saved into biblio for fields (a), (b),
    (d) and (e)
11/ modify the same order line, check that values you've filled is
    correctly retrieved.
12/ modify all values, save, and check biblio once again
13/ create a new order line on the same biblio used for previous order
    line. check that values are correctly retrieved from biblio

Signed-off-by: Harold Dramer <harold.dramer@nyls.edu>
Comment 23 Jesse Weaver 2016-07-21 20:50:05 UTC
Created attachment 53615 [details] [review]
Bug 11844: Copy additional fields values when transfering order

Signed-off-by: Harold Dramer <harold.dramer@nyls.edu>
Comment 24 Jesse Weaver 2016-07-21 20:50:11 UTC
Created attachment 53616 [details] [review]
Bug 11844: Fix additional field values when creating a new order line

ordernumber was not correctly retrieved

Signed-off-by: Harold Dramer <harold.dramer@nyls.edu>
Comment 25 Jesse Weaver 2016-07-21 20:50:51 UTC Comment hidden (obsolete)
Comment 26 Jesse Weaver 2016-07-21 21:55:01 UTC
Created attachment 53619 [details] [review]
Bug 11844: (followup) use new unified additional field screen
Comment 27 Jonathan Druart 2016-07-22 09:18:31 UTC
Global symbol "$additional_fields" requires explicit package name (did you forget to declare "my $additional_fields"?) at acqui/neworderempty.pl line 313.
Comment 28 Julian Maurice 2016-09-13 13:33:42 UTC
Created attachment 55529 [details] [review]
Bug 11844: Fix syntax errors
Comment 29 Jonathan Druart 2016-09-14 14:09:25 UTC
Blocked by bug 15774.