Bug 15685

Created attachment 55096 [details] [review]
Bug 15685: Allow creation of items (AcqCreateItem) to be customizable per-basket

This adds a new basket attribute (create_items) that can optionally be
set to override AcqCreateItem.

The following have been modified to reflect this (with the value of
create_items that causes them to behave differently in parentheses):
  * Cancelling receipt of an order (receiving)
  * Creating an order by hand or from MARC (ordering)
  * Receiving an order (receiving)
  * Showing orders with uncertain price (ordering)
  * Showing orders (receiving)
  * Showing acquisition details in the OPAC (ordering)

Test plan:
  1) Create baskets with "Create items when:" set to ordering,
     receiving, cataloging and unset.
  2) Test each of the above for each of these baskets, verifying that
     the basket-specific attribute overrides AcqCreateItem if set and
     falls back to the syspref otherwise.

NOTE: A check of AcqCreateItem in opac-detail.tt was removed because it
was redundant; the code path in question cannot be triggered unless
create_items/AcqCreateItems is set to the correct value anyway.

I used PTFS test set up to create test budget; test Ebsco vendor and baskets. The System Preference by default for AcqCreateItem is set to create an item when "placing an order"...mainpage says updgrade to 16.05 done....

Test 1. 2016_0902 basket had the basket's Create items when set to "when placing an order". Results: Success in terms of when creating a new item is prompted correctly upon adding a title to a new basket. This is the same as the default system preference, however....

Test 2. Basket's 'create items when' option is set to make an item when "receiving an order"... Results: Fail. I tried to skip adding an item record at the point of adding a new title to a test basket. I went to Accounting details to put in order record information. However, I was blocked on the QUANTITY field. It stayed at zero and didn't allow for a numeral (e.g., 1, 2). Once I click Add item record above that area, then the Quantity field gets filled in as '1' and I can save the newly ordered title. 
I closed the 2016_0902Test2 basket and hit receive shipment. I receive one title in the basket.... Software error screen appears....Can't locate object method "find" via package "Koha::Acquisition::Order" at /home/koha/src/acqui/orderreceive.pl line 113.
I hit F5, refresh, then try to click text link to receive other title. Same thing.

Test 3. New test basket has the 'create items when' option set to 'cataloging the record'... Results: same thing happened as in test 2. I next close that basket. Receive shipment on 89.00 dollar book. Software error. Can't locate object method "find" via package "Koha::Acquisition::Order" at /home/koha/src/acqui/orderreceive.pl line 113. 

Seems to fail but I'll try another sandbox soon. Hesistant to change the status yet in case my test plan was off. jls.

(In reply to J Schmidt from comment #4)
> I used PTFS test set up to create test budget; test Ebsco vendor and
> baskets. The System Preference by default for AcqCreateItem is set to create
> an item when "placing an order"...mainpage says updgrade to 16.05 done....
> 
> Test 1. 2016_0902 basket had the basket's Create items when set to "when
> placing an order". Results: Success in terms of when creating a new item is
> prompted correctly upon adding a title to a new basket. This is the same as
> the default system preference, however....
> 
> Test 2. Basket's 'create items when' option is set to make an item when
> "receiving an order"... Results: Fail. I tried to skip adding an item record
> at the point of adding a new title to a test basket. I went to Accounting
> details to put in order record information. However, I was blocked on the
> QUANTITY field. It stayed at zero and didn't allow for a numeral (e.g., 1,
> 2). Once I click Add item record above that area, then the Quantity field
> gets filled in as '1' and I can save the newly ordered title. 
> I closed the 2016_0902Test2 basket and hit receive shipment. I receive one
> title in the basket.... Software error screen appears....Can't locate object
> method "find" via package "Koha::Acquisition::Order" at
> /home/koha/src/acqui/orderreceive.pl line 113.
> I hit F5, refresh, then try to click text link to receive other title. Same
> thing.
> 
> Test 3. New test basket has the 'create items when' option set to
> 'cataloging the record'... Results: same thing happened as in test 2. I next
> close that basket. Receive shipment on 89.00 dollar book. Software error.
> Can't locate object method "find" via package "Koha::Acquisition::Order" at
> /home/koha/src/acqui/orderreceive.pl line 113. 
> 
> Seems to fail but I'll try another sandbox soon. Hesistant to change the
> status yet in case my test plan was off. jls.

In each case I had to create a brief, new bib record. couldn't locate a biblionumber from test database. That would be a better thing to use upon adding new titles to a basket....

Hi Jenny,

I think the problem may have been that the schema needs to be updated in order to fully test this patch. 

I had similar results to yours, but after updating the schema (not possible on a sandbox) things worked correctly

-Nick

Okay. I'll work on another bug then based on what you are sharing here.. Thanks.

Would love to see this happen.

Created attachment 57390 [details] [review]
Bug 15685: Allow creation of items (AcqCreateItem) to be customizable per-basket

This adds a new basket attribute (create_items) that can optionally be
set to override AcqCreateItem.

The following have been modified to reflect this (with the value of
create_items that causes them to behave differently in parentheses):
  * Cancelling receipt of an order (receiving)
  * Creating an order by hand or from MARC (ordering)
  * Receiving an order (receiving)
  * Showing orders with uncertain price (ordering)
  * Showing orders (receiving)
  * Showing acquisition details in the OPAC (ordering)

Test plan:
  1) Create baskets with "Create items when:" set to ordering,
     receiving, cataloging and unset.
  2) Test each of the above for each of these baskets, verifying that
     the basket-specific attribute overrides AcqCreateItem if set and
     falls back to the syspref otherwise.

NOTE: A check of AcqCreateItem in opac-detail.tt was removed because it
was redundant; the code path in question cannot be triggered unless
create_items/AcqCreateItems is set to the correct value anyway.

Created attachment 57392 [details] [review]
Bug 15685: Allow creation of items (AcqCreateItem) to be customizable per-basket

This adds a new basket attribute (create_items) that can optionally be
set to override AcqCreateItem.

The following have been modified to reflect this (with the value of
create_items that causes them to behave differently in parentheses):
  * Cancelling receipt of an order (receiving)
  * Creating an order by hand or from MARC (ordering)
  * Receiving an order (receiving)
  * Showing orders with uncertain price (ordering)
  * Showing orders (receiving)
  * Showing acquisition details in the OPAC (ordering)

Test plan:
  1) Create baskets with "Create items when:" set to ordering,
     receiving, cataloging and unset.
  2) Test each of the above for each of these baskets, verifying that
     the basket-specific attribute overrides AcqCreateItem if set and
     falls back to the syspref otherwise.

NOTE: A check of AcqCreateItem in opac-detail.tt was removed because it
was redundant; the code path in question cannot be triggered unless
create_items/AcqCreateItems is set to the correct value anyway.

Created attachment 57395 [details] [review]
Bug 15685: Allow creation of items (AcqCreateItem) to be customizable per-basket

This adds a new basket attribute (create_items) that can optionally be
set to override AcqCreateItem.

The following have been modified to reflect this (with the value of
create_items that causes them to behave differently in parentheses):
  * Cancelling receipt of an order (receiving)
  * Creating an order by hand or from MARC (ordering)
  * Receiving an order (receiving)
  * Showing orders with uncertain price (ordering)
  * Showing orders (receiving)
  * Showing acquisition details in the OPAC (ordering)

Test plan:
  1) Create baskets with "Create items when:" set to ordering,
     receiving, cataloging and unset.
  2) Test each of the above for each of these baskets, verifying that
     the basket-specific attribute overrides AcqCreateItem if set and
     falls back to the syspref otherwise.

NOTE: A check of AcqCreateItem in opac-detail.tt was removed because it
was redundant; the code path in question cannot be triggered unless
create_items/AcqCreateItems is set to the correct value anyway.

Created attachment 57424 [details] [review]
Bug 15685: Allow creation of items (AcqCreateItem) to be customizable per-basket

This adds a new basket attribute (create_items) that can optionally be
set to override AcqCreateItem.

The following have been modified to reflect this (with the value of
create_items that causes them to behave differently in parentheses):
  * Cancelling receipt of an order (receiving)
  * Creating an order by hand or from MARC (ordering)
  * Receiving an order (receiving)
  * Showing orders with uncertain price (ordering)
  * Showing orders (receiving)
  * Showing acquisition details in the OPAC (ordering)

Test plan:
  1) Create baskets with "Create items when:" set to ordering,
     receiving, cataloging and unset.
  2) Test each of the above for each of these baskets, verifying that
     the basket-specific attribute overrides AcqCreateItem if set and
     falls back to the syspref otherwise.

NOTE: A check of AcqCreateItem in opac-detail.tt was removed because it
was redundant; the code path in question cannot be triggered unless
create_items/AcqCreateItems is set to the correct value anyway.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Barbara Fondren <bfondren@roundrocktexas.gov>

Patch set doesn't take Edifact ordering into account.

Created attachment 57425 [details] [review]
Bug 15685: Allow creation of items (AcqCreateItem) to be customizable per-basket

This adds a new basket attribute (create_items) that can optionally be
set to override AcqCreateItem.

The following have been modified to reflect this (with the value of
create_items that causes them to behave differently in parentheses):
  * Cancelling receipt of an order (receiving)
  * Creating an order by hand or from MARC (ordering)
  * Receiving an order (receiving)
  * Showing orders with uncertain price (ordering)
  * Showing orders (receiving)
  * Showing acquisition details in the OPAC (ordering)

Test plan:
  1) Create baskets with "Create items when:" set to ordering,
     receiving, cataloging and unset.
  2) Test each of the above for each of these baskets, verifying that
     the basket-specific attribute overrides AcqCreateItem if set and
     falls back to the syspref otherwise.

NOTE: A check of AcqCreateItem in opac-detail.tt was removed because it
was redundant; the code path in question cannot be triggered unless
create_items/AcqCreateItems is set to the correct value anyway.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Barbara Fondren <bfondren@roundrocktexas.gov>

Created attachment 57426 [details] [review]
Bug 15685 [QA Followup] - Implement needed changes in Koha::Edifact::Order

Created attachment 57427 [details] [review]
Bug 15685 [QA Followup] - Bug qa script errors

Tests are missing.

Comment on attachment 57425 [details] [review]
Bug 15685: Allow creation of items (AcqCreateItem) to be customizable per-basket

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

::: installer/data/mysql/kohastructure.sql
@@ +2964,4 @@
>    `basketgroupid` int(11), -- links this basket to its group (aqbasketgroups.id)
>    `deliveryplace` varchar(10) default NULL, -- basket delivery place
>    `billingplace` varchar(10) default NULL, -- basket billing place
> +  create_items ENUM('ordering', 'receiving', 'cataloguing') default NULL; -- when items should be created for orders in this basket

The semicolon needs to be a comma, and an atomicupdate needs to be added.

Created attachment 64918 [details] [review]
Bug 15685 - QA Followup

DB revision fixes
Unit tests

Created attachment 64919 [details] [review]
Bug 15685: Allow creation of items (AcqCreateItem) to be customizable per-basket

This adds a new basket attribute (create_items) that can optionally be
set to override AcqCreateItem.

The following have been modified to reflect this (with the value of
create_items that causes them to behave differently in parentheses):
  * Cancelling receipt of an order (receiving)
  * Creating an order by hand or from MARC (ordering)
  * Receiving an order (receiving)
  * Showing orders with uncertain price (ordering)
  * Showing orders (receiving)
  * Showing acquisition details in the OPAC (ordering)

Test plan:
  1) Create baskets with "Create items when:" set to ordering,
     receiving, cataloging and unset.
  2) Test each of the above for each of these baskets, verifying that
     the basket-specific attribute overrides AcqCreateItem if set and
     falls back to the syspref otherwise.

NOTE: A check of AcqCreateItem in opac-detail.tt was removed because it
was redundant; the code path in question cannot be triggered unless
create_items/AcqCreateItems is set to the correct value anyway.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Barbara Fondren <bfondren@roundrocktexas.gov>

Created attachment 64920 [details] [review]
Bug 15685 [QA Followup] - Implement needed changes in Koha::Edifact::Order

Created attachment 64921 [details] [review]
Bug 15685 [QA Followup] - Bug qa script errors

Created attachment 64922 [details] [review]
Bug 15685 - QA Followup

DB revision fixes
Unit tests

Hi, very interested to see this one make it in. Took a look at the code and noticed some merge markers were left in:

b/acqui/finishreceive.pl
+<<<<<<< HEAD
 $unitprice = Koha::Number::Price->new( $unitprice )->unformat();
 
 #need old receivedate if we update the order, parcel.pl only shows the right parcel this way FIXME
+=======

Created attachment 65119 [details] [review]
Bug 15685: Allow creation of items (AcqCreateItem) to be customizable per-basket

This adds a new basket attribute (create_items) that can optionally be
set to override AcqCreateItem.

The following have been modified to reflect this (with the value of
create_items that causes them to behave differently in parentheses):
  * Cancelling receipt of an order (receiving)
  * Creating an order by hand or from MARC (ordering)
  * Receiving an order (receiving)
  * Showing orders with uncertain price (ordering)
  * Showing orders (receiving)
  * Showing acquisition details in the OPAC (ordering)

Test plan:
  1) Create baskets with "Create items when:" set to ordering,
     receiving, cataloging and unset.
  2) Test each of the above for each of these baskets, verifying that
     the basket-specific attribute overrides AcqCreateItem if set and
     falls back to the syspref otherwise.

NOTE: A check of AcqCreateItem in opac-detail.tt was removed because it
was redundant; the code path in question cannot be triggered unless
create_items/AcqCreateItems is set to the correct value anyway.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Barbara Fondren <bfondren@roundrocktexas.gov>

Created attachment 65120 [details] [review]
Bug 15685 [QA Followup] - Implement needed changes in Koha::Edifact::Order

Created attachment 65121 [details] [review]
Bug 15685 [QA Followup] - Bug qa script errors

Created attachment 65122 [details] [review]
Bug 15685 - QA Followup

DB revision fixes
Unit tests

Still trying to give this a second sign-off as I think this will make life a lot easier. Sadly, I ran into a big problem:

AcqCreateItem is set to: on order

1) The method create_items is not covered by tests!
- Create a new basket, choose: on receive
- Add to basket by searching for an existing record in your catalog
- Click order on one of the results
=> Internal server error, translates to: The method create_items is not covered by tests!

Can't test further from here :(

2) The test file should not be named Basket.pm, but Basket.t :)

3) Patch reintroduced a typo (not really blocking)
-#need old receivedate if we update the order, parcel.pl only shows the right parcel this way FIXME
+#need old recievedate if we update the order, parcel.pl only shows the right parcel this way FIXME


Tiny stuff (definitely non-blocker, maybe for later):
- Baskets created before the introduction of the feature don't show the information about item creation on the basket summary page
- Editing a basket after inititally creating it, doesn't offer the option to change. This makes a lot of sense, but maybe it would be good to display the selection as information (not changeable).

Absolutely love the feauture!
This makes acquisitions so much more flexible especially for all the cases where you don't want to create items, like for electronic resources. Can't want to get my hands on it.

Created attachment 67638 [details] [review]
Bug 15685: Allow creation of items (AcqCreateItem) to be customizable per-basket

This adds a new basket attribute (create_items) that can optionally be
set to override AcqCreateItem.

The following have been modified to reflect this (with the value of
create_items that causes them to behave differently in parentheses):
  * Cancelling receipt of an order (receiving)
  * Creating an order by hand or from MARC (ordering)
  * Receiving an order (receiving)
  * Showing orders with uncertain price (ordering)
  * Showing orders (receiving)
  * Showing acquisition details in the OPAC (ordering)

Test plan:
  1) Create baskets with "Create items when:" set to ordering,
     receiving, cataloging and unset.
  2) Test each of the above for each of these baskets, verifying that
     the basket-specific attribute overrides AcqCreateItem if set and
     falls back to the syspref otherwise.

NOTE: A check of AcqCreateItem in opac-detail.tt was removed because it
was redundant; the code path in question cannot be triggered unless
create_items/AcqCreateItems is set to the correct value anyway.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Barbara Fondren <bfondren@roundrocktexas.gov>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 67639 [details] [review]
Bug 15685: Implement needed changes in Koha::Edifact::Order

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 67640 [details] [review]
Bug 15685: (QA followup) Fix qa script errors

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 67641 [details] [review]
Bug 15685: QA Followup

DB revision fixes
Unit tests

Edit: fixed the update step description (tcohen)

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 67642 [details] [review]
Bug 15685: Schema changes

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 67643 [details] [review]
Bug 15685: (QA followup) Fix typo

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 67644 [details] [review]
Bug 15685: (followup) K:A:O->find and ->fetch are no longer used

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 67645 [details] [review]
Bug 15685: (QA followup) Tidy tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 67646 [details] [review]
Bug 15685: POD

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 67647 [details] [review]
Bug 15685: Add tests for Koha::Acquisition::Order->basket

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

I've rebased the patches and checked everyting is ok regarding the changes introduced by bug 19256 in Koha::Acquisition::Order(s).

Added tests for Koha::Acquisition::Order::basket (found a bug in the meantime).

Please re-test.

Created attachment 67689 [details] [review]
Bug 15685: Allow creation of items (AcqCreateItem) to be customizable per-basket

This adds a new basket attribute (create_items) that can optionally be
set to override AcqCreateItem.

The following have been modified to reflect this (with the value of
create_items that causes them to behave differently in parentheses):
  * Cancelling receipt of an order (receiving)
  * Creating an order by hand or from MARC (ordering)
  * Receiving an order (receiving)
  * Showing orders with uncertain price (ordering)
  * Showing orders (receiving)
  * Showing acquisition details in the OPAC (ordering)

Test plan:
  1) Create baskets with "Create items when:" set to ordering,
     receiving, cataloging and unset.
  2) Test each of the above for each of these baskets, verifying that
     the basket-specific attribute overrides AcqCreateItem if set and
     falls back to the syspref otherwise.

NOTE: A check of AcqCreateItem in opac-detail.tt was removed because it
was redundant; the code path in question cannot be triggered unless
create_items/AcqCreateItems is set to the correct value anyway.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Barbara Fondren <bfondren@roundrocktexas.gov>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 67690 [details] [review]
Bug 15685: Implement needed changes in Koha::Edifact::Order

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 67691 [details] [review]
Bug 15685: (QA followup) Fix qa script errors

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 67692 [details] [review]
Bug 15685: QA Followup

DB revision fixes
Unit tests

Edit: fixed the update step description (tcohen)

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 67693 [details] [review]
Bug 15685: Schema changes

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 67694 [details] [review]
Bug 15685: (QA followup) Fix typo

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 67695 [details] [review]
Bug 15685: (followup) K:A:O->find and ->fetch are no longer used

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 67696 [details] [review]
Bug 15685: (QA followup) Tidy tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 67697 [details] [review]
Bug 15685: POD

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 67698 [details] [review]
Bug 15685: Add tests for Koha::Acquisition::Order->basket

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Some adjustments are required:
1. Update DB entry must use column_exists()
2. t/db_dependent/Acquisition/CancelReceipt.t and t/db_dependent/Acquisition/TransferOrder.t are failing with
  Can't use string ("Koha::Acquisition::Order") as a HASH ref while "strict refs" in use at /home/vagrant/kohaclone/Koha/Object.pm line 275.
3. Koha::EDI and circ/reserveratios.pl are still using AcqCreateItem from the pref
4. On basketheader.pl?booksellerid=1&op=add_form, the values for the dropdown "Create items when:" ended with a '.'
I'd say it should be removed or moved out the options.
5. This looks like a typo (basketheader.tt):
-                        <input type="hidden" name="is_an_edit" value="1" />
+                        <input type="hidden" name="rs_an_edit" value="1" />
6. Koha::Acquisition::Order->basket must use DBIx::Class rs
7. Double check the code in addorderiso2709, it is buggy (GetBasket and Koha::Baskets->find calls)
Can't call method "effective_create_items" on unblessed reference at /home/vagrant/kohaclone/acqui/addorderiso2709.pl line 107.
8. Change in acqui/basket.pl is not needed (basket passed twice to the template)

Created attachment 67884 [details] [review]
Bug 15685: Followup - address QA issues

Created attachment 67898 [details] [review]
Bug 15685: Allow creation of items (AcqCreateItem) to be customizable per-basket

This adds a new basket attribute (create_items) that can optionally be
set to override AcqCreateItem.

The following have been modified to reflect this (with the value of
create_items that causes them to behave differently in parentheses):
  * Cancelling receipt of an order (receiving)
  * Creating an order by hand or from MARC (ordering)
  * Receiving an order (receiving)
  * Showing orders with uncertain price (ordering)
  * Showing orders (receiving)
  * Showing acquisition details in the OPAC (ordering)

Test plan:
  1) Create baskets with "Create items when:" set to ordering,
     receiving, cataloging and unset.
  2) Test each of the above for each of these baskets, verifying that
     the basket-specific attribute overrides AcqCreateItem if set and
     falls back to the syspref otherwise.

NOTE: A check of AcqCreateItem in opac-detail.tt was removed because it
was redundant; the code path in question cannot be triggered unless
create_items/AcqCreateItems is set to the correct value anyway.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Barbara Fondren <bfondren@roundrocktexas.gov>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 67899 [details] [review]
Bug 15685: Implement needed changes in Koha::Edifact::Order

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 67900 [details] [review]
Bug 15685: (QA followup) Fix qa script errors

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 67901 [details] [review]
Bug 15685: QA Followup

DB revision fixes
Unit tests

Edit: fixed the update step description (tcohen)

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 67902 [details] [review]
Bug 15685: (QA followup) Fix typo

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 67903 [details] [review]
Bug 15685: (followup) K:A:O->find and ->fetch are no longer used

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 67904 [details] [review]
Bug 15685: (QA followup) Tidy tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 67905 [details] [review]
Bug 15685: POD

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 67906 [details] [review]
Bug 15685: Add tests for Koha::Acquisition::Order->basket

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 67907 [details] [review]
Bug 15685: Followup - address QA issues

Created attachment 67908 [details] [review]
Bug 15685: Fix DB columns order

Created attachment 67909 [details] [review]
Bug 15685: DBIC Schema changes

Comment on attachment 67907 [details] [review]
Bug 15685: Followup - address QA issues

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

-    my $basket = Koha::Acquisition::Order->find( $order->{ordernumber} )->basket;
+    my $basket = Koha::Acquisition::Orders->find({ordernumber => $ordernumber})->basket;

Why?

my $basket = Koha::Acquisition::Orders->find($ordernumber)->basket;

should be enough.

Created attachment 67910 [details] [review]
Bug 15685: Avoid "Use of uninitialized value" warning in logs

Created attachment 67911 [details] [review]
Bug 15685: Send the basket to _cancel_items_receipt

(In reply to Jonathan Druart from comment #82)
> Created attachment 67911 [details] [review] [review]
> Bug 15685: Send the basket to _cancel_items_receipt

Nick, does this last patch makes sense to you?

(In reply to Jonathan Druart from comment #83)
> Nick, does this last patch makes sense to you?

Yes, there is more we could switch to use of objects, but I didn't want to get sidetracked

(In reply to Jonathan Druart from comment #80)
> my $basket = Koha::Acquisition::Orders->find($ordernumber)->basket;
> 
> should be enough.
Indeed - I was debugging a typo so was more specific to be sure error was not there

Pushed to master for 17.11, thanks to everybody involved!

There's a typo in this:

Koha/EDI.pm - $basket->effective_create_item (Missing the 's'), which will cause Koha::Objects to throw an 'The method is untested' error whenever his codepath is followed.