Bug 35398 - EDI: Fix support for LRP (Library Rotation Plan) for Koha with Stock Rotation enabled
Summary: EDI: Fix support for LRP (Library Rotation Plan) for Koha with Stock Rotation...
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Martin Renvoize
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on: 20595
Blocks:
  Show dependency treegraph
 
Reported: 2023-11-24 13:06 UTC by Martin Renvoize
Modified: 2024-03-19 15:22 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.04,23.05.10


Attachments
Bug 35398: Fix LRP support for EDI orders with single items (1.42 KB, patch)
2023-11-24 13:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35398: Fix LRP support for EDI orders with single items (1.49 KB, patch)
2023-12-28 11:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35398: Fix LRP support for EDI orders with single items (1.49 KB, patch)
2023-12-28 12:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35398: Fix LRP support for EDI orders with single items (1.55 KB, patch)
2024-01-02 14:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35398: (QA follow-up) Tidy code (1.58 KB, patch)
2024-01-02 14:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35398: Add unit test for extraction of library_rotation_plan (15.21 KB, patch)
2024-02-28 16:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35398: Unit test (5.84 KB, patch)
2024-02-28 16:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35398: Fix LRP support for EDI orders with single items (1.54 KB, patch)
2024-02-28 16:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35398: (QA follow-up) Tidy code (1.58 KB, patch)
2024-02-28 16:45 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2023-11-24 13:06:02 UTC
Bug 20595 introduced support for LRP fields in EDI with StockRotation enabled.  However, it missed a case where each order line only contains one item per line.

This wasn't spotted in the existing uses as they structure their orders somewhat differently, but is being highlighted at a sight where they regularly order in this way.
Comment 1 Martin Renvoize 2023-11-24 13:08:08 UTC
Created attachment 159240 [details] [review]
Bug 35398: Fix LRP support for EDI orders with single items

This patch corrects a mistake in the original implementaiton of bug
20595 where we introduced support for the LRP segments being used to
automatically assign items ordered via EDI to stock rotation plans.
Comment 2 Martin Renvoize 2023-12-28 11:34:58 UTC
This is now in production at a number of PTFS-E sites.
Comment 3 Martin Renvoize 2023-12-28 11:36:25 UTC
Created attachment 160350 [details] [review]
Bug 35398: Fix LRP support for EDI orders with single items

This patch corrects a mistake in the original implementaiton of bug
20595 where we introduced support for the LRP segments being used to
automatically assign items ordered via EDI to stock rotation plans.

Signed-off-by: ophie Halden <sophie.halden@cheshiresharedservices.gov.uk>
Comment 4 Martin Renvoize 2023-12-28 11:36:54 UTC
Signoff line added on behalf of customer using fix in production.
Comment 5 Martin Renvoize 2023-12-28 12:03:05 UTC
Created attachment 160352 [details] [review]
Bug 35398: Fix LRP support for EDI orders with single items

This patch corrects a mistake in the original implementaiton of bug
20595 where we introduced support for the LRP segments being used to
automatically assign items ordered via EDI to stock rotation plans.

Signed-off-by: Sophie Halden <sophie.halden@cheshiresharedservices.gov.uk>
Comment 6 Kyle M Hall 2024-01-02 14:11:41 UTC
Created attachment 160406 [details] [review]
Bug 35398: Fix LRP support for EDI orders with single items

This patch corrects a mistake in the original implementaiton of bug
20595 where we introduced support for the LRP segments being used to
automatically assign items ordered via EDI to stock rotation plans.

Signed-off-by: Sophie Halden <sophie.halden@cheshiresharedservices.gov.uk>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 7 Kyle M Hall 2024-01-02 14:11:51 UTC
Created attachment 160407 [details] [review]
Bug 35398: (QA follow-up) Tidy code

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Katrin Fischer 2024-01-31 21:33:55 UTC
I'd feel much better if we had some unit tests here. What do you think?
Comment 9 Martin Renvoize 2024-02-08 14:48:27 UTC
It's not at all trivial to write a test for this.. it's in production at all PTFS-E sites backported to 22.11.x and beyond.

Whilst I still agree tests are good, EDI has been notoriously difficult to write tests for from day one.
Comment 10 Katrin Fischer 2024-02-27 16:42:22 UTC
I asked and as it's a bug fix, I am open to make an exception here.

I cannot really test EDI unless I am provided with data and a detailed test plan. I think it's similar for others, so we might rely more on code review here than elsewhere. I think in the long term unit tests would be really helpful to avoid breaking things and make us more confident about making changes. I know EDIFACT is a critical feature for a lot of libraries.

As you know, pushing might still take a bit for reasons, but I'll switch back to PQA.
Comment 11 Martin Renvoize 2024-02-28 16:45:07 UTC
Created attachment 162541 [details] [review]
Bug 35398: Add unit test for extraction of library_rotation_plan

This unit test addition adds an LRP segment to the test EDI message file
and adds the corresponding test to confirm is it extracted into the
field as expected.
Comment 12 Martin Renvoize 2024-02-28 16:45:09 UTC
Created attachment 162542 [details] [review]
Bug 35398: Unit test

This patch adds the begginings of unit tests for Koha::EDI.  Right now
we only test that a simple QUOTE message creates a basket, adds the item
to said basket and assigns that single item to a corresponding stock
rotation rota as defined by the LRP segment in the QUOTE message.

It lays the foundations for much more rigorous tests to be written to
cover the whole of EDI.pm however.
Comment 13 Martin Renvoize 2024-02-28 16:45:11 UTC
Created attachment 162543 [details] [review]
Bug 35398: Fix LRP support for EDI orders with single items

This patch corrects a mistake in the original implementaiton of bug
20595 where we introduced support for the LRP segments being used to
automatically assign items ordered via EDI to stock rotation plans.

Signed-off-by: Sophie Halden <sophie.halden@cheshiresharedservices.gov.uk>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 14 Martin Renvoize 2024-02-28 16:45:14 UTC
Created attachment 162544 [details] [review]
Bug 35398: (QA follow-up) Tidy code

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 15 Martin Renvoize 2024-02-28 16:47:25 UTC
Two full days of writing code later I've finally managed to get to grips with writing the very basics of a unit test for this.  Considering the original patch took 20 minutes I'm a bit bewildered by that.. but at least it's a start of unit tests for this area.

I now also have a parse/deparse pair of scripts for taking an EDI message and making it a bit more human readable and then doing the opposite to package up an edited version.. that's how I created the test file added here.  Those scripts are not included here for now.

Anyway.. I think we are good to go here now.
Comment 16 Katrin Fischer 2024-03-07 13:39:56 UTC
Fixed the file permission error and tidied the new file EDI.t (amended commits). Decided to ignore the one line on t/Edifact.t for now.

 WARN	t/Edifact.t
   WARN	  tidiness
		The file is less tidy than before (bad/messy lines before: 15, now: 16)

 FAIL	t/db_dependent/Koha/EDI.t
   FAIL	  file permissions
		File must have the exec flag
   WARN	  tidiness
		The file is less tidy than before (bad/messy lines before: 0, now: 7)
Comment 17 Katrin Fischer 2024-03-07 14:03:55 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 18 Fridolin Somers 2024-03-11 09:37:13 UTC
Pushed to 23.11.x for 23.11.04
Comment 19 Lucas Gass 2024-03-19 15:22:34 UTC
Backported to 23.05.x for upcoming 23.05.10.