Processing of Edifact quotes aborts with runtime error Can't use string ("Koha::Acquisition::Basket") as a HASH ref while "strict refs" in use at /usr/share/koha/lib/Koha/Object.pm line 275. Problem introduced as a result of an inappropriate change introduced as part of bug 15685
NB Basket will have been created with value defaulting to the syspref
Created attachment 73090 [details] [review] proposed patch restoring working code Restores the existing working code that used the system preference. The per-basket setting has been set based on that systempreference when loading the quote which this orderline is part, so the checking the value was redundant at this point
While this change is simple enough: 1) Where's the test plan? (as if I have no clue what Edifact quotes are and how to trigger them -- which, sadly, I do.) 2) Why is there no use Koha::Acquisition::Basket? 3) Is it supposed to be Basket or Baskets? 4) As such, is this the correct way to fix the problem?
Where was the test when this code was introduced to break EDI? Basically people cant run Edifact EDI with the addition of this code It undoes a change that should never have been made
Created attachment 73103 [details] [review] Bug 20446: QUOTES processing broken by run time error
(In reply to Colin Campbell from comment #4) > Where was the test when this code was introduced to break EDI? A lack of a test does not obviate our responsibility to add regression tests where possible and reasonable. Adding unit tests while fixing regressions in Koha is a very common occurrence. I do understand the frustration as I've been in this situation dozens of times ; ) I notice that there unit test files for invoices and orders, but not quotes. Is there any reason quotes weren't unit tested when introduced? I'll leave it to cait of mtompsett to decide if unit tests are necessary. > Basically people cant run Edifact EDI with the addition of this code > It undoes a change that should never have been made Your patch seems to introduce a regression of its own. Baskets in Koha can now override AcqCreateItem with their own alternative setting. The patch I've submitted should take care of the issue and keep support for the new feature!
Kyle your patch is illogical the baskets in question CANNOT override the syspreference, because we have just created them in accordance with that syspreference microseconmds before. Baskets can override the systempreference if there is user interaction to make that choice, the whole point of electronic ordering is that there is no such interaction and the basket is generated from the QUOTES message directly with no user interaction. reinstanting the original patch
Created attachment 73104 [details] [review] proposed patch to allow users to use acquisitions via EDI
(In reply to Colin Campbell from comment #7) > Kyle your patch is illogical the baskets in question CANNOT override the > syspreference, because we have just created them in accordance with that > syspreference microseconmds before. > Baskets can override the systempreference if there is user interaction to > make that choice, the whole point of electronic ordering is that there is no > such interaction and the basket is generated from the QUOTES message > directly with no user interaction. > reinstanting the original patch But his change allows for the case where there IS user interaction. And if it was created microseconds before, the values should be identical. If it is user interaction later, why should a system preference change after the order creation change the behaviour? Once the order is created, should you not change the behaviour on the order, not forcefully via system preference? This is why tests are very much needed, because then we'd know how it is supposed to behave, because the tests would demonstrate trigger cases. Marking Failed QA until I see tests.
There is and cannot be a user interaction - you bare processing a file there is no user satr at a terminal.
(In reply to Colin Campbell from comment #10) > There is and cannot be a user interaction - you bare processing a file there > is no user satr at a terminal. This is an offline process.
My mistake. Colin is correct. misc/cronjobs/edi_cron.pl calls process_quote() in Koha::EDI, which calls quote_item in Koha::EDI. As far as I can tell there is no other way to trigger quote_item. And so, I believe the original patch was more correct, in that it is simpler. So apologies for any frustrations. *IF* in the future, someone wanted to manually integrate EDI quote processing -- it would be useful to see debugging feedback step by step -- Kyle's patch makes sense. However, I still believe there should be test coverage. There is no test coverage of Koha::EDI. This needs to be solved. It is a serious hole. Test coverage for quote_item and/or process_quote would be good, leaving Failed QA.
(In reply to M. Tompsett from comment #12) > My mistake. Colin is correct. misc/cronjobs/edi_cron.pl calls > process_quote() in Koha::EDI, which calls quote_item in Koha::EDI. As far as > I can tell there is no other way to trigger quote_item. And so, I believe > the original patch was more correct, in that it is simpler. So apologies for > any frustrations. > > *IF* in the future, someone wanted to manually integrate EDI quote > processing -- it would be useful to see debugging feedback step by step -- > Kyle's patch makes sense. > > However, I still believe there should be test coverage. There is no test > coverage of Koha::EDI. This needs to be solved. It is a serious hole. Test > coverage for quote_item and/or process_quote would be good, leaving Failed > QA. patch reinstates a call to C4::Context - do we need another test to show that works? QA failed not for this patch, but for more global concerns, I'd argur thats counterproductive - or shall we just remove this functionality from Koha
We can ask why no tests was submitted initially OR when the new feature was added, but this doesn't change that this is a critical bug and we broke an important acquisitions workflow. Tomorrow releases might happen already. If we want to have a chance to get this included, we need to move fast. Can someone verify the change now is correct and sign off? I suggest documenting the need for tests on a separate bug for now. Even better of course, if someone provides them.
Katrin makes a valid point. Sign off now, tests later. I didn't officially sign off, because I just shut down my work computer where I was signing off. But I'll set to Signed Off, because it truly is an identical reversion.
(In reply to Colin Campbell from comment #7) > Kyle your patch is illogical the baskets in question CANNOT override the > syspreference, because we have just created them in accordance with that > syspreference microseconmds before. > Baskets can override the systempreference if there is user interaction to > make that choice, the whole point of electronic ordering is that there is no > such interaction and the basket is generated from the QUOTES message > directly with no user interaction. > reinstanting the original patch I see you are correct, I hadn't read the calling code where the basket was created. My bad!
Kyle, can you QA please?
Created attachment 73156 [details] [review] Bug 20446 Revert to using syspref for item creation Using a per-basket setting is nonsensical when baskets are created automatically by quote processing a few lines before this attempts to read it. cron job aborts with a runtime error leaving basket for quote half-created. This reverts to the previous code using the global setting. (Basket itself will have been set to the global setting by default) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Caused by commit 04aea91de0f2fe1103e4021f880d135da1fd11a9 Bug 15685: (QA follow-up) Address QA issues
The original problem is that we are calling ->find on Koha::Object instead of the set class (Koha::Objects) - my $basket = Koha::Acquisition::Basket->find( $basketno ); + my $basket = Koha::Acquisition::Baskets->find( $basketno ); Sounds like the correct fix to me. Could someone confirm?
(In reply to Jonathan Druart from comment #20) > The original problem is that we are calling ->find on Koha::Object instead > of the set class (Koha::Objects) > > - my $basket = Koha::Acquisition::Basket->find( $basketno ); > + my $basket = Koha::Acquisition::Baskets->find( $basketno ); > > Sounds like the correct fix to me. Could someone confirm? Read the above comments. There is no need to retrieve the basket which you have just created. It was being retrieved to find a parameter that was set based on the systempreference. It introduce3s extra processing for no purpose
It's more about consistency: AcqCreateItem pref's value should not be retrieved directly, the logic should be stay in Koha::Acquisition::Basket
Picking this for 17.11.04 to restore functionality - will watch here for final resolution and update as needed
Bug 15685 not in 17.05.x
(In reply to Jonathan Druart from comment #22) > It's more about consistency: AcqCreateItem pref's value should not be > retrieved directly, the logic should be stay in Koha::Acquisition::Basket Both solutions do the job, but given the fact we have an encapsulated business-logic behind 'effective_create_item', the best way to do it is to use the encapsulated one, because any changes on that business logic or (new?) pref combinations wouldn't have an impact on the EDI code itself.
(In reply to Tomás Cohen Arazi from comment #25) > (In reply to Jonathan Druart from comment #22) > > It's more about consistency: AcqCreateItem pref's value should not be > > retrieved directly, the logic should be stay in Koha::Acquisition::Basket > > Both solutions do the job, but given the fact we have an encapsulated > business-logic behind 'effective_create_item', the best way to do it is to > use the encapsulated one, because any changes on that business logic or > (new?) pref combinations wouldn't have an impact on the EDI code itself. Its a pity I'm not seeing regression tests for this btw.
Created attachment 73424 [details] [review] Bug 20446: Fix Edifact quotes processing Caused by commit 04aea91de0f2fe1103e4021f880d135da1fd11a9 Bug 15685: (QA follow-up) Address QA issues ->find is called on Koha::Object instead of the set class (Koha::Objects) and raises the following error: Can't use string ("Koha::Acquisition::Basket") as a HASH ref while "strict refs" in use at /usr/share/koha/lib/Koha/Object.pm line 275. This patch also makes sure $basketno refers to an existing basket in DB I cannot provide a test plan, I have no idea how this code is used.
I have attached an alternative patch for this issue. Could you please test and confirm it makes sense? I have tried but failed writing tests for this change.
Its not sensible it adds a lot of code to test something we already have the result of. The only basket info we want is the basketno, returned on creation which is passed as a parameter Add unnecessary code is surely potentially buggy
Created attachment 73736 [details] [review] Bug 20446: Fix Edifact quotes processing Caused by commit 04aea91de0f2fe1103e4021f880d135da1fd11a9 Bug 15685: (QA follow-up) Address QA issues ->find is called on Koha::Object instead of the set class (Koha::Objects) and raises the following error: Can't use string ("Koha::Acquisition::Basket") as a HASH ref while "strict refs" in use at /usr/share/koha/lib/Koha/Object.pm line 275. This patch also makes sure $basketno refers to an existing basket in DB I cannot provide a test plan, I have no idea how this code is used. Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Any finds in quote processing are redundant logic is Create basket if return is ok [; create orderline create orderline ]; The find was introduced on a misconception.
Created attachment 73756 [details] [review] Bug 20446: Fix Edifact quotes processing Caused by commit 04aea91de0f2fe1103e4021f880d135da1fd11a9 Bug 15685: (QA follow-up) Address QA issues ->find is called on Koha::Object instead of the set class (Koha::Objects) and raises the following error: Can't use string ("Koha::Acquisition::Basket") as a HASH ref while "strict refs" in use at /usr/share/koha/lib/Koha/Object.pm line 275. This patch also makes sure $basketno refers to an existing basket in DB I cannot provide a test plan, I have no idea how this code is used. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Colin Campbell from comment #31) > Any finds in quote processing are redundant > > logic is > > Create basket > if return is ok [; > create orderline > create orderline > ]; > > The find was introduced on a misconception. Yes, for now. But the logic could change later. Nowadays I have more concerns about consistency/maintainability rather than performance. Do not read this wrong: we are talking about a select on a primary key, so very limited impact.
Pushed to master for 18.05, thanks to everybody involved!
Already in 17.11.x, marking as RESOLVED FIXED.