|
Description
Martin Renvoize (ashimema)
2023-11-24 13:06:02 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. This is now in production at a number of PTFS-E sites. 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> Signoff line added on behalf of customer using fix in production. 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> 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> Created attachment 160407 [details] [review] Bug 35398: (QA follow-up) Tidy code Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> I'd feel much better if we had some unit tests here. What do you think? 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. 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. 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. 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. 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> Created attachment 162544 [details] [review] Bug 35398: (QA follow-up) Tidy code Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> 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. 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) Pushed for 24.05! Well done everyone, thank you! Pushed to 23.11.x for 23.11.04 Backported to 23.05.x for upcoming 23.05.10. |