Bug 29670 - Restore functionality broken by bug 27708 for AcqCreateItem set to "placing an order"
Summary: Restore functionality broken by bug 27708 for AcqCreateItem set to "placing a...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low blocker (vote)
Assignee: Martin Renvoize
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on: 27708
Blocks:
  Show dependency treegraph
 
Reported: 2021-12-09 13:52 UTC by Martin Renvoize
Modified: 2022-12-12 21:24 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This patch restores the lost GIR segments in EDI messages generated by orders with items attached.
Version(s) released in:
22.05.00,21.11.02,21.05.11,20.11.15


Attachments
Bug 29670: Fix EDI for AcqCreateItem = 'placing on order' (1.27 KB, patch)
2021-12-09 13:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29670: Fix EDI for AcqCreateItem = 'placing on order' (1.56 KB, patch)
2021-12-09 15:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29670: Unit tests (7.31 KB, patch)
2021-12-10 11:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29670: Fix EDI for AcqCreateItem = 'placing on order' (1.64 KB, patch)
2021-12-10 11:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29670: Unit tests (7.37 KB, patch)
2021-12-10 11:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29670: Fix EDI for AcqCreateItem = 'placing on order' (1.70 KB, patch)
2021-12-10 11:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29670: Unit tests (7.48 KB, patch)
2021-12-12 14:43 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29670: Fix EDI for AcqCreateItem = 'placing on order' (1.76 KB, patch)
2021-12-12 14:43 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29670: Unit tests (7.52 KB, patch)
2022-01-18 16:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29670: Fix EDI for AcqCreateItem = 'placing on order' (1.76 KB, patch)
2022-01-18 16:42 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2021-12-09 13:52:14 UTC
Bug 27708 misunderstood how the acquisitions link tables work and thus incorrectly assigned values from rows in the wrong table.
Comment 1 Martin Renvoize 2021-12-09 13:57:07 UTC
Created attachment 128375 [details] [review]
Bug 29670: Fix EDI for AcqCreateItem = 'placing on order'

The AcqCreatItem at order time functionality was broken by bug 27708.
This patch resolves that.
Comment 2 Martin Renvoize 2021-12-09 15:20:20 UTC
Created attachment 128379 [details] [review]
Bug 29670: Fix EDI for AcqCreateItem = 'placing on order'

The AcqCreatItem at order time functionality was broken by bug 27708.
This patch resolves that.
Comment 3 Martin Renvoize 2021-12-10 11:09:09 UTC
Created attachment 128391 [details] [review]
Bug 29670: Unit tests

This patch adds unit tests for Koha::Edifact::Order->order_line. We now
check that the message segments are created as expected for both the
'ordering' and not 'ordering' case for acquisitions item creation time.
Comment 4 Martin Renvoize 2021-12-10 11:09:13 UTC
Created attachment 128392 [details] [review]
Bug 29670: Fix EDI for AcqCreateItem = 'placing on order'

The AcqCreatItem at order time functionality was broken by bug 27708.
This patch resolves that.

Test plan.
1) Run the newly created unit tests that prove both settings work
Comment 5 Martin Renvoize 2021-12-10 11:13:32 UTC
Created attachment 128393 [details] [review]
Bug 29670: Unit tests

This patch adds unit tests for Koha::Edifact::Order->order_line. We now
check that the message segments are created as expected for both the
'ordering' and not 'ordering' case for acquisitions item creation time.

Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Comment 6 Martin Renvoize 2021-12-10 11:13:36 UTC
Created attachment 128394 [details] [review]
Bug 29670: Fix EDI for AcqCreateItem = 'placing on order'

The AcqCreatItem at order time functionality was broken by bug 27708.
This patch resolves that.

Test plan.
1) Run the newly created unit tests that prove both settings work

Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Comment 7 Martin Renvoize 2021-12-10 11:17:07 UTC
Tested on live environments that exhibited problems prior to the patch and signed off.

The issue was that if your settings meant that items were created at order time, but 27708 made it so that the GIR segments would not get sent to the vendor.

The patch fixes that problem, but leaves in place the handling for when items are not created at order time.  I also corrected the itemtype assignment key for the case where items are not created as this was incorrect in bug 27708 too.
Comment 8 Katrin Fischer 2021-12-12 14:42:56 UTC
I can only partially test this, but I have made sure the unit tests pass and I have run through the test plan from bug 27708. No errors in the GUI and none in the logs.
Comment 9 Katrin Fischer 2021-12-12 14:43:33 UTC
Created attachment 128419 [details] [review]
Bug 29670: Unit tests

This patch adds unit tests for Koha::Edifact::Order->order_line. We now
check that the message segments are created as expected for both the
'ordering' and not 'ordering' case for acquisitions item creation time.

Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 10 Katrin Fischer 2021-12-12 14:43:38 UTC
Created attachment 128420 [details] [review]
Bug 29670: Fix EDI for AcqCreateItem = 'placing on order'

The AcqCreatItem at order time functionality was broken by bug 27708.
This patch resolves that.

Test plan.
1) Run the newly created unit tests that prove both settings work

Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 11 Martin Renvoize 2021-12-13 16:07:24 UTC
Thanks Katrin, the yeah, EDI is notoriously difficult to test.  I'm really pleased with the unit tests this patch adds personally, I feel like it's a real steel in the right direction for keeping EDI working in future versions
Comment 12 Fridolin Somers 2021-12-22 07:02:32 UTC
Ah QA tests fail for me :
 FAIL	t/db_dependent/Koha/Edifact/Order.t
   FAIL	  spelling
		 segement  ==> segment
		 segement  ==> segment
Comment 13 Jonathan Druart 2022-01-18 16:41:02 UTC
(In reply to Fridolin Somers from comment #12)
> Ah QA tests fail for me :
>  FAIL	t/db_dependent/Koha/Edifact/Order.t
>    FAIL	  spelling
> 		 segement  ==> segment
> 		 segement  ==> segment

Critical bug stuck because of that?
Comment 14 Jonathan Druart 2022-01-18 16:42:32 UTC
Created attachment 129584 [details] [review]
Bug 29670: Unit tests

This patch adds unit tests for Koha::Edifact::Order->order_line. We now
check that the message segments are created as expected for both the
'ordering' and not 'ordering' case for acquisitions item creation time.

Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

JD amended patch: spelling segement ==> segment
Comment 15 Jonathan Druart 2022-01-18 16:42:36 UTC
Created attachment 129585 [details] [review]
Bug 29670: Fix EDI for AcqCreateItem = 'placing on order'

The AcqCreatItem at order time functionality was broken by bug 27708.
This patch resolves that.

Test plan.
1) Run the newly created unit tests that prove both settings work

Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 16 Fridolin Somers 2022-01-19 07:21:00 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 17 Martin Renvoize 2022-01-21 16:48:28 UTC
This affects 21.11.00,21.05.05 and 20.11.12.. it needs backporting
Comment 18 Kyle M Hall 2022-01-25 12:01:48 UTC
Pushed to 21.11.x for 21.11.02
Comment 19 Andrew Fuerste-Henry 2022-02-10 15:47:19 UTC
Pushed to 21.05.x for 21.05.11
Comment 20 Victor Grousset/tuxayo 2022-02-12 20:33:54 UTC
This one is tricky for 20.11.X

The patch fixes one of the tests but there are others failing.

Digging the differences with 21.05.x showed that bug 27793 wasn't on 20.11.x and bingo, that's what makes the difference in output and fixes the tests :D

So I'll backport it to have this one. 27793 doesn't seem too scary since no regression has been found after all this time and the missing automated tests are coming with this very ticket. And the functional change shouldn't bother users.

An alternative is reverting bug 27708

Tell me in the next week (before the release) if the alternative would actually be better.
Comment 21 Victor Grousset/tuxayo 2022-02-12 20:36:13 UTC
Backported: Pushed to 20.11.x branch for 20.11.15
Comment 22 wainuiwitikapark 2022-02-15 04:00:55 UTC
Do I need to backport this to 19.11.x? I probably won't unless there is a need for it
Comment 23 Victor Grousset/tuxayo 2022-02-16 17:19:00 UTC
It depends on bug 27708 so no need for 19.11.x
Comment 24 wainuiwitikapark 2022-02-18 00:05:47 UTC
Not backported to 19.11.x