Description
Joonas Kylmälä
2021-02-15 14:24:22 UTC
Created attachment 117453 [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 and then treated as one, but in the former item data was stored as hash keys in the same variable so later it was treated like an object, hence why it crashed. 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. Created attachment 117455 [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. 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 Created attachment 117568 [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. 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> 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. 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 The tests are needed still for this to pass QA, so moving to FQA status. (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. Created attachment 117722 [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. Created attachment 117723 [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> Created attachment 127227 [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 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> 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> 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. 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> 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> 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> Passing QA as all the patches have the required sign-offs. Pushed to master for 21.11, thanks to everybody involved! Pushed to 21.05.x for 21.05.05 Pushed to 20.11.x for 20.11.12 Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed. 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 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 |