Bug 38423 - EDIFACT invoice files should skip orders that cannot be receipted rather than failing to complete
Summary: EDIFACT invoice files should skip orders that cannot be receipted rather than...
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low major
Assignee: Martin Renvoize (ashimema)
QA Contact: Kyle M Hall (khall)
URL:
Keywords:
Depends on: 22415
Blocks: 38689
  Show dependency treegraph
 
Reported: 2024-11-12 11:40 UTC by Lucy Vaux-Harvey
Modified: 2025-01-28 02:08 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes loading EDIFACT invoice files so that it skips a problematic order (usually a cancelled order or a deleted bibliographic or item record), reports any problem orders, and completes the processing of other orders. Previously, the EDIFACT page would get "stuck" and display as "Processing" for problematic orders, then the remainder of the orders in the file had to be manually receipted by library staff (as vendors are reluctant to re-process part invoices).
Version(s) released in:
25.05.00
Circulation function:


Attachments
Bug 38423: Code more defensively in process_invoice (9.84 KB, patch)
2024-11-12 12:43 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38423: Code more defensively in process_invoice (9.72 KB, patch)
2024-11-12 12:57 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38423: Code more defensively in process_invoice (9.79 KB, patch)
2024-12-06 15:57 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38423: Code more defensively in process_invoice (9.77 KB, patch)
2024-12-12 15:24 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38423: Add unit tests (5.40 KB, patch)
2024-12-12 15:24 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38423: Code more defensively in process_invoice (9.77 KB, patch)
2024-12-12 15:26 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38423: Add unit tests (6.42 KB, patch)
2024-12-12 15:26 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38423: Code more defensively in process_invoice (9.83 KB, patch)
2024-12-16 02:11 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 38423: Add unit tests (6.47 KB, patch)
2024-12-16 02:11 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 38423: Add unit tests (6.53 KB, patch)
2025-01-10 14:46 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lucy Vaux-Harvey 2024-11-12 11:40:54 UTC
When loading an EDIFACT invoice file, if the process finds a problematic order (usually a cancelled order or a deleted bibliographic or item record) the load process gets stuck and displays as 'Processing' in the EDFACT messages page. This usually means that the remainder of the orders in the file have to be manually receipted by library staff as vendors are reluctant to re-process part invoices.

It would be preferable if the process could skip, and report any problem orders but complete all other receipts.
Comment 1 Martin Renvoize (ashimema) 2024-11-12 12:43:36 UTC
Created attachment 174406 [details] [review]
Bug 38423: Code more defensively in process_invoice

We re-arrange the logic of process_invoice a little here to ensure we
skip order lines in invoices that do not have corresponding bib records.
Comment 2 Martin Renvoize (ashimema) 2024-11-12 12:57:50 UTC
Created attachment 174408 [details] [review]
Bug 38423: Code more defensively in process_invoice

We re-arrange the logic of process_invoice a little here to ensure we
skip order lines in invoices that do not have corresponding bib records.
Comment 3 Victor Grousset/tuxayo 2024-11-29 06:05:30 UTC
Any resource for how to test this? Or a test plan?

After trying find resources on EDI for Koha it seems the easiest path would be:

> If you set the EdifactInvoiceImport preference to ‘Don’t’ you can load invoice files manually.

But I don't know where that should be uploaded ^^"
Or maybe it's about having them in "EDIFACT messages" and manually clicking on loading them.

Then I think I would need matching order and invoice message files to test.

There doesn't seem to be an EDIFACT test plan in the whole git history :o
To know what's the minimal setup for EDI accounts to just pick files locally from a directory.

And there is no t/db_dependent/Koha/Edifact/Invoice.t what would help test at a lower level.

Only t/db_dependent/Koha/Edifact/Order.t
Comment 4 Martin Renvoize (ashimema) 2024-12-04 10:07:51 UTC
Editfact is hard to test as you need test edifact files.. I tend to rely heavily on Kyle to look at these ones to date as he's one of the few remaining people here with the knowledge of such messages to review these.
Comment 5 Kyle M Hall (khall) 2024-12-06 15:57:19 UTC
Created attachment 175267 [details] [review]
Bug 38423: Code more defensively in process_invoice

We re-arrange the logic of process_invoice a little here to ensure we
skip order lines in invoices that do not have corresponding bib records.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 6 Victor Grousset/tuxayo 2024-12-10 18:24:51 UTC
Is it possible to keep such test messages somewhere so anyone needing to test EDIFACT stuff can use them? After changing their content if there is record data that is intellectual property of a vendor.

So I could signoff and Kyle could QA. Otherwise we might be a bit stuck.
Comment 7 Martin Renvoize (ashimema) 2024-12-12 15:24:57 UTC
Created attachment 175405 [details] [review]
Bug 38423: Code more defensively in process_invoice

We re-arrange the logic of process_invoice a little here to ensure we
skip order lines in invoices that do not have corresponding bib records.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Martin Renvoize (ashimema) 2024-12-12 15:24:59 UTC
Created attachment 175406 [details] [review]
Bug 38423: Add unit tests

Sponsored-by: PTFS Europe <https://ptfs-europe.com>
Comment 9 Martin Renvoize (ashimema) 2024-12-12 15:26:20 UTC
Created attachment 175407 [details] [review]
Bug 38423: Code more defensively in process_invoice

We re-arrange the logic of process_invoice a little here to ensure we
skip order lines in invoices that do not have corresponding bib records.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Martin Renvoize (ashimema) 2024-12-12 15:26:23 UTC
Created attachment 175408 [details] [review]
Bug 38423: Add unit tests

Sponsored-by: PTFS Europe <https://ptfs-europe.com>
Comment 11 Martin Renvoize (ashimema) 2024-12-12 15:27:19 UTC
Unit tests added and these include an example INVOICE.CEI file with cases for each of the failures we now skip over with this patch.
Comment 12 Victor Grousset/tuxayo 2024-12-16 02:11:26 UTC
Created attachment 175485 [details] [review]
Bug 38423: Code more defensively in process_invoice

We re-arrange the logic of process_invoice a little here to ensure we
skip order lines in invoices that do not have corresponding bib records.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 13 Victor Grousset/tuxayo 2024-12-16 02:11:29 UTC
Created attachment 175486 [details] [review]
Bug 38423: Add unit tests

Sponsored-by: PTFS Europe <https://ptfs-europe.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 14 Victor Grousset/tuxayo 2024-12-16 02:16:03 UTC
It works! :)

Thanks Martin for the tests :D

When having only the test commit applied (minor conflict on the `carp "Cannot find[..]`), the test fail with:

"Can't call method "biblionumber" on an undefined value"

That's in line with the implementation being about defensive coding.
Comment 15 Kyle M Hall (khall) 2025-01-10 14:46:38 UTC
Created attachment 176344 [details] [review]
Bug 38423: Add unit tests

Sponsored-by: PTFS Europe <https://ptfs-europe.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 16 Katrin Fischer 2025-01-10 18:20:57 UTC
Pushed for 25.05!

Well done everyone, thank you!