Bug 27708 - Cannot create EDI order if AcqCreateItem value is not "placing an order"
Summary: Cannot create EDI order if AcqCreateItem value is not "placing an order"
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Peter Vashchuk
QA Contact: Testopia
URL:
Keywords:
Depends on: 23926
Blocks: 29670
  Show dependency treegraph
 
Reported: 2021-02-15 14:24 UTC by Joonas Kylmälä
Modified: 2021-12-09 13:52 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.05,20.11.12


Attachments
Bug 27708: unify two item object creation blocks to be stored as hash (3.06 KB, patch)
2021-03-01 14:20 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 27708: unify two item object creation blocks to be stored as hash (3.11 KB, patch)
2021-03-01 14:26 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 27708: unify two item object creation blocks to be stored as hash (3.29 KB, patch)
2021-03-03 12:35 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 27708: unify two item object creation blocks to be stored as hash (3.33 KB, patch)
2021-03-04 10:01 UTC, David Nind
Details | Diff | Splinter Review
Bug 27708: Unify two item object creation blocks to be stored as hash (3.29 KB, patch)
2021-03-04 13:35 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 27708: Unify two item object creation blocks to be stored as hash (3.33 KB, patch)
2021-03-04 13:40 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 27708: (QA follow-up) Add a few tests for Koha::Edifact::Order (4.10 KB, patch)
2021-11-02 23:43 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27708: Unify two item object creation blocks to be stored as hash (3.48 KB, patch)
2021-11-02 23:44 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27708: (QA follow-up) Add a few tests for Koha::Edifact::Order (4.25 KB, patch)
2021-11-02 23:44 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27708: Unify two item object creation blocks to be stored as hash (3.52 KB, patch)
2021-11-03 09:27 UTC, David Nind
Details | Diff | Splinter Review
Bug 27708: (QA follow-up) Add a few tests for Koha::Edifact::Order (4.30 KB, patch)
2021-11-03 09:27 UTC, David Nind
Details | Diff | Splinter Review
Bug 27708: (QA follow-up) Add a few tests for Koha::Edifact::Order (4.35 KB, patch)
2021-11-03 14:06 UTC, Peter Vashchuk
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Joonas Kylmälä 2021-02-15 14:24:22 UTC
There was a regression in bug 23926 which causes EDI orders to give internal server error if AcqCreateItem syspref is set to something other than "placing an order." The code from patch "Bug 23926: Limit GIR segment to 5 pieces of info" (f9efa7a9) assumes in the line

> branchcode     => $item->homebranch->branchcode,

that we have an Koha object but that is not true when AcqCreateItem is set to not create an item while placing the order. In that case we manually create hash representing item which has the key "branch" that contains the branchcode so the code "$item->homebranch->branchcode" doesn't work because it should be $item{branch}.
Comment 1 Peter Vashchuk 2021-03-01 14:20:39 UTC Comment hidden (obsolete)
Comment 2 Peter Vashchuk 2021-03-01 14:26:22 UTC Comment hidden (obsolete)
Comment 3 Joonas Kylmälä 2021-03-02 09:47:32 UTC
Thanks for the patch! I think 

> $item_hash->{branch} = $branch;

Would need to be changed to 

> $item_hash->{branchcode} = $branch;

The delivery location info won't get transmitted otherwise.

Can you please also add test for

a) delivery place

b) Some tests for when AcqCreateItem is different value
Comment 4 Peter Vashchuk 2021-03-03 12:35:35 UTC Comment hidden (obsolete)
Comment 5 David Nind 2021-03-04 10:01:20 UTC
Created attachment 117665 [details] [review]
Bug 27708: unify two item object creation blocks to be stored as hash

Previously for existing item data was stored as an object reference and
then treated as one, but for not yet existing item data was stored as
keys in hash reference in the same variable and later it was treated
like an object, hence why it crashed with "no method".
This patch unifies that variable in both cases filled as hash and
treated as such.

To reproduce:
    1) Go to "Administration->System preferences" and change
"AcqCreateItem" to "receiving an order."
    2) Now, go to "Acquisitions" and create a new Vendor,
or use an existing one.
    3) Next, go to "Administration->EDI Account" and add EDI account
(pick that Vendor that you created recently, or the one that you will
use for this test).
    4) Also in "Administration->Library EANs" add EAN if you didn't
have one previously.
    5) Go back to "Acquisitions" and add a new basket to your Vendor
that you will use for this test.
    6) Press "Create EDIFACT order" button. It should throw
"Can't call method "homebranch" on unblessed reference..."
software error.
    7) Apply the patch.
    8) Reload the page that threw software error previously
(or repeat steps 5, 6 if you need another basket),
it should go through now.

Signed-off-by: David Nind <david@davidnind.com>
Comment 6 David Nind 2021-03-04 10:06:00 UTC
Thanks for the nice test plan Peter!

I've never dealt with EDI,so it was great to have a nice straight forward walk through.

Testing notes:
- Add an item to the basket in step 5 to generate the error.
Comment 7 David Nind 2021-03-04 10:07:57 UTC
Also, you may want to fix the Bug title to help it get through QA - after the Bug XXXX: it needs to start with a capital letter:

Bug XXXX: Title of the bug
Comment 8 Joonas Kylmälä 2021-03-04 10:27:05 UTC
The tests are needed still for this to pass QA, so moving to FQA status.
Comment 9 Peter Vashchuk 2021-03-04 13:32:50 UTC
(In reply to David Nind from comment #6)
> Thanks for the nice test plan Peter!
> 
> I've never dealt with EDI,so it was great to have a nice straight forward
> walk through.

Hey, thanks for the sign-off!

> Testing notes:
> - Add an item to the basket in step 5 to generate the error.

I didn't add anything about adding the item to the basket because you are
able to sight the error even without adding anything to that basket, all you 
need to do is to press "Create EDIFACT order" which is available even if basket
is empty.
Comment 10 Peter Vashchuk 2021-03-04 13:35:19 UTC Comment hidden (obsolete)
Comment 11 Peter Vashchuk 2021-03-04 13:40:19 UTC Comment hidden (obsolete)
Comment 12 Joonas Kylmälä 2021-11-02 23:43:27 UTC Comment hidden (obsolete)
Comment 13 Joonas Kylmälä 2021-11-02 23:44:39 UTC
Created attachment 127228 [details] [review]
Bug 27708: Unify two item object creation blocks to be stored as hash

Previously for existing item data was stored as an object reference and
then treated as one, but for not yet existing item data was stored as
keys in hash reference in the same variable and later it was treated
like an object, hence why it crashed with "no method".
This patch unifies that variable in both cases filled as hash and
treated as such.

To reproduce:
    1) Go to "Administration->System preferences" and change
"AcqCreateItem" to "receiving an order."
    2) Now, go to "Acquisitions" and create a new Vendor,
or use an existing one.
    3) Next, go to "Administration->EDI Account" and add EDI account
(pick that Vendor that you created recently, or the one that you will
use for this test).
    4) Also in "Administration->Library EANs" add EAN if you didn't
have one previously.
    5) Go back to "Acquisitions" and add a new basket to your Vendor
that you will use for this test.
    6) Press "Create EDIFACT order" button. It should throw
"Can't call method "homebranch" on unblessed reference..."
software error.
    7) Apply the patch.
    8) Reload the page that threw software error previously
(or repeat steps 5, 6 if you need another basket),
it should go through now.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 14 Joonas Kylmälä 2021-11-02 23:44:43 UTC
Created attachment 127229 [details] [review]
Bug 27708: (QA follow-up) Add a few tests for Koha::Edifact::Order

1. This adds a simple regression test to make sure order_line() method
executes correctly with basket create_items set to "ordering" and
"receiving".

2. A simple test for the filename method is added

To test:
 1) prove t/db_dependent/Koha/Edifact/Order.t

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 15 Joonas Kylmälä 2021-11-02 23:46:55 UTC
Added the missing unit test to make this pass QA, however the test is quite big so I would like to have separate sign-off and qa sign-off for the unit test patch. You can count my sign-off for the first patch as QA sign-off.
Comment 16 David Nind 2021-11-03 09:27:32 UTC
Created attachment 127233 [details] [review]
Bug 27708: Unify two item object creation blocks to be stored as hash

Previously for existing item data was stored as an object reference and
then treated as one, but for not yet existing item data was stored as
keys in hash reference in the same variable and later it was treated
like an object, hence why it crashed with "no method".
This patch unifies that variable in both cases filled as hash and
treated as such.

To reproduce:
    1) Go to "Administration->System preferences" and change
"AcqCreateItem" to "receiving an order."
    2) Now, go to "Acquisitions" and create a new Vendor,
or use an existing one.
    3) Next, go to "Administration->EDI Account" and add EDI account
(pick that Vendor that you created recently, or the one that you will
use for this test).
    4) Also in "Administration->Library EANs" add EAN if you didn't
have one previously.
    5) Go back to "Acquisitions" and add a new basket to your Vendor
that you will use for this test.
    6) Press "Create EDIFACT order" button. It should throw
"Can't call method "homebranch" on unblessed reference..."
software error.
    7) Apply the patch.
    8) Reload the page that threw software error previously
(or repeat steps 5, 6 if you need another basket),
it should go through now.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: David Nind <david@davidnind.com>
Comment 17 David Nind 2021-11-03 09:27:51 UTC
Created attachment 127234 [details] [review]
Bug 27708: (QA follow-up) Add a few tests for Koha::Edifact::Order

1. This adds a simple regression test to make sure order_line() method
executes correctly with basket create_items set to "ordering" and
"receiving".

2. A simple test for the filename method is added

To test:
 1) prove t/db_dependent/Koha/Edifact/Order.t

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: David Nind <david@davidnind.com>
Comment 18 Peter Vashchuk 2021-11-03 14:06:13 UTC
Created attachment 127264 [details] [review]
Bug 27708: (QA follow-up) Add a few tests for Koha::Edifact::Order

1. This adds a simple regression test to make sure order_line() method
executes correctly with basket create_items set to "ordering" and
"receiving".

2. A simple test for the filename method is added

To test:
 1) prove t/db_dependent/Koha/Edifact/Order.t

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>
Comment 19 Joonas Kylmälä 2021-11-03 18:13:05 UTC
Passing QA as all the patches have the required sign-offs.
Comment 20 Jonathan Druart 2021-11-05 11:14:42 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 21 Kyle M Hall 2021-11-11 12:29:33 UTC
Pushed to 21.05.x for 21.05.05
Comment 22 Fridolin Somers 2021-11-12 23:16:34 UTC
Pushed to 20.11.x for 20.11.12
Comment 23 Victor Grousset/tuxayo 2021-11-16 08:15:37 UTC
Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed.
Comment 24 Martin Renvoize 2021-12-09 13:47:08 UTC
Comment on attachment 127233 [details] [review]
Bug 27708: Unify two item object creation blocks to be stored as hash

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

::: Koha/Edifact/Order.pm
@@ +392,5 @@
> +                    branchcode     => $item->{branchcode},
> +                    itype          => $item->{itype},
> +                    location       => $item->{location},
> +                    itemcallnumber => $item->{itemcallnumber},
> +                };

Pretty sure the above block causes breakage.. you are using `$item` as though it's a hash representation of an item row.. but it's not.. it's the hash of the AqOrdersItems link table.. so it doesn't contain branchcode, itype, location or itemcallnumber.

It should be using the actual item row.. as before.. $i_obj

This breaks our live systems
Comment 25 Martin Renvoize 2021-12-09 13:47:11 UTC
Comment on attachment 127233 [details] [review]
Bug 27708: Unify two item object creation blocks to be stored as hash

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

::: Koha/Edifact/Order.pm
@@ +392,5 @@
> +                    branchcode     => $item->{branchcode},
> +                    itype          => $item->{itype},
> +                    location       => $item->{location},
> +                    itemcallnumber => $item->{itemcallnumber},
> +                };

Pretty sure the above block causes breakage.. you are using `$item` as though it's a hash representation of an item row.. but it's not.. it's the hash of the AqOrdersItems link table.. so it doesn't contain branchcode, itype, location or itemcallnumber.

It should be using the actual item row.. as before.. $i_obj

This breaks our live systems