Bug 20446 - QUOTES processing broken by run time error
Summary: QUOTES processing broken by run time error
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 15685
Blocks:
  Show dependency treegraph
 
Reported: 2018-03-19 10:41 UTC by Colin Campbell
Modified: 2018-04-06 17:53 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
proposed patch restoring working code (1.20 KB, patch)
2018-03-19 11:14 UTC, Colin Campbell
Details | Diff | Splinter Review
Bug 20446: QUOTES processing broken by run time error (963 bytes, patch)
2018-03-20 13:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
proposed patch to allow users to use acquisitions via EDI (1.20 KB, patch)
2018-03-20 14:21 UTC, Colin Campbell
Details | Diff | Splinter Review
Bug 20446 Revert to using syspref for item creation (1.27 KB, patch)
2018-03-22 11:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20446: Fix Edifact quotes processing (1.97 KB, patch)
2018-03-29 13:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20446: Fix Edifact quotes processing (2.02 KB, patch)
2018-04-06 06:30 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20446: Fix Edifact quotes processing (2.11 KB, patch)
2018-04-06 09:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Colin Campbell 2018-03-19 10:41:38 UTC
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
Comment 1 Colin Campbell 2018-03-19 11:09:38 UTC
NB Basket will have been created with value defaulting to the syspref
Comment 2 Colin Campbell 2018-03-19 11:14:05 UTC
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
Comment 3 M. Tompsett 2018-03-19 14:05:24 UTC
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?
Comment 4 Colin Campbell 2018-03-19 14:12:00 UTC
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
Comment 5 Kyle M Hall 2018-03-20 13:11:53 UTC
Created attachment 73103 [details] [review]
Bug 20446: QUOTES processing broken by run time error
Comment 6 Kyle M Hall 2018-03-20 13:20:14 UTC
(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!
Comment 7 Colin Campbell 2018-03-20 14:19:48 UTC
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
Comment 8 Colin Campbell 2018-03-20 14:21:00 UTC
Created attachment 73104 [details] [review]
proposed patch to allow users to use acquisitions via EDI
Comment 9 M. Tompsett 2018-03-20 15:57:56 UTC
(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.
Comment 10 Colin Campbell 2018-03-20 16:53:39 UTC
There is and cannot be a user interaction - you bare processing a file there is no user satr at a terminal.
Comment 11 Colin Campbell 2018-03-20 16:55:57 UTC
(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.
Comment 12 M. Tompsett 2018-03-20 18:15:50 UTC
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.
Comment 13 Colin Campbell 2018-03-20 18:36:18 UTC
(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
Comment 14 Katrin Fischer 2018-03-21 06:08:29 UTC
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.
Comment 15 M. Tompsett 2018-03-21 06:21:45 UTC
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.
Comment 16 Kyle M Hall 2018-03-21 11:11:32 UTC
(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!
Comment 17 Katrin Fischer 2018-03-22 06:15:05 UTC
Kyle, can you QA please?
Comment 18 Kyle M Hall 2018-03-22 11:10:45 UTC
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>
Comment 19 Jonathan Druart 2018-03-23 19:12:51 UTC
Caused by
  commit 04aea91de0f2fe1103e4021f880d135da1fd11a9
  Bug 15685: (QA follow-up) Address QA issues
Comment 20 Jonathan Druart 2018-03-23 19:19:25 UTC
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?
Comment 21 Colin Campbell 2018-03-26 08:11:01 UTC
(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
Comment 22 Jonathan Druart 2018-03-26 14:02:59 UTC
It's more about consistency: AcqCreateItem pref's value should not be retrieved directly, the logic should be stay in Koha::Acquisition::Basket
Comment 23 Nick Clemens 2018-03-26 14:40:41 UTC
Picking this for 17.11.04 to restore functionality - will watch here for final resolution and update as needed
Comment 24 Fridolin SOMERS 2018-03-27 11:55:32 UTC
Bug 15685 not in 17.05.x
Comment 25 Tomás Cohen Arazi 2018-03-28 18:24:27 UTC
(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.
Comment 26 Tomás Cohen Arazi 2018-03-28 18:26:59 UTC
(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.
Comment 27 Jonathan Druart 2018-03-29 13:43:02 UTC
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.
Comment 28 Jonathan Druart 2018-03-29 13:45:48 UTC
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.
Comment 29 Colin Campbell 2018-03-29 14:38:09 UTC
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
Comment 30 Josef Moravec 2018-04-06 06:30:08 UTC
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>
Comment 31 Colin Campbell 2018-04-06 08:57:04 UTC
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.
Comment 32 Marcel de Rooy 2018-04-06 09:30:50 UTC
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>
Comment 33 Jonathan Druart 2018-04-06 13:44:28 UTC
(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.
Comment 34 Jonathan Druart 2018-04-06 17:53:43 UTC
Pushed to master for 18.05, thanks to everybody involved!