Bug 21417

Summary: EDI ordering fails when basket and EAN libraries do not match
Product: Koha Reporter: Andrew Fuerste-Henry <andrewfh>
Component: AcquisitionsAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: normal    
Priority: P5 - low CC: colin.campbell, fridolin.somers, kyle, martin.renvoize, nick, pierre-marc.thibault
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 21417: Fix the warning lines to output the basketno
Bug 21417: EDI ordering fails when basket and EAN libraries do not match
Bug 21417: EDI ordering fails when basket and EAN libraries
Supplemental patch to display the null entry
Bug 21417 - Display default EAN as such in admin display
Bug 21417: EDI ordering fails when basket and EAN libraries do not match
Bug 21417: (QA follow-up) Display default EAN as such in admin display

Description Andrew Fuerste-Henry 2018-09-26 16:30:18 UTC
When clicking Create EDIFACT Order on /acqui/basket.pl, an EDIFACT message will not be generated if aqbasket.branch does not match edifact_ean.branchcode. This failure does not generate any sort of error message, it just fails to produce a message.

This bug is complicated by a couple things:
- When setting up an EAN, the user is forced to select a branch. There is not an option for all branches.
- When creating a basket, the branch defaults to the library the user is currently logged into. This branch cannot be changed at basket creation, only after the basket has been created.
Comment 1 Kyle M Hall 2018-09-26 16:34:56 UTC
When this situation is triggered, the error 
"No sender ean passed to order creation: basket = $self->{basket}->basketno()";
is written to the log. It should be noted the use  "$self->{basket}->basketno()" does not get interpolated correctly and should be moved outside the string.
Comment 2 Kyle M Hall 2018-09-26 16:37:21 UTC
Not sure what the correct fix here is. There are a number of options:
A) Strip EANs from the pulldown that don't match the basket's branchcode
B) Ignore the branchcode for the purpose of the search
   i) Add option for 'all libraries' to EAN editor?
C) Halt with a screen error

I suppose we could implement all of the above, but really only one should be sufficient.
Comment 3 Colin Campbell 2018-09-26 19:14:54 UTC
Must admit that I've been wondering whether the branch - ean link is really useful. We inherited that from the original EDI implementation that envisioned a situation where the system may have had multiple eans. Thats not a case we've encountered in practice. Wonder if there should be a global all libraries default, and a branch requiring a different san/ean would be the special case.
Comment 4 Andrew Fuerste-Henry 2018-09-26 19:33:06 UTC
A bit of correction to my original comment: I'd thought that aqbasket.branch was auto-populating with the branch the user is logged into. In fact, aqbasket.branch is null by default. The dropdown with which one would change it to specify a branch gives the current branch as a default option, but does not actually set it. This explains why we haven't seen this error more often and makes it a bit less dire -- the librarian needs to do a little extra work to make the system break.
Comment 5 Colin Campbell 2018-09-26 19:39:39 UTC
I'm thinking there should be a default line in the config, which should cope with null as well. We should fall back to that if exists if no library-specific entry exists.
Comment 6 Kyle M Hall 2018-09-27 11:04:35 UTC
Created attachment 79475 [details] [review]
Bug 21417: Fix the warning lines to output the basketno
Comment 7 Kyle M Hall 2018-09-27 11:04:43 UTC
Created attachment 79476 [details] [review]
Bug 21417: EDI ordering fails when basket and EAN libraries do not match

When clicking Create EDIFACT Order on /acqui/basket.pl, an EDIFACT message will not be generated if aqbasket.branch does not match edifact_ean.branchcode. This failure does not generate any sort of error message, it just fails to produce a message.

We should allow ean's to not require a branch to be set, then if we don't find a branch specific each, we can look for the default version of the ean.

Test Plan:
1) Apply this patch set
2) Run updatedatabase
3) Verify you can create a Library EAN without setting a branchcode for it
4) Verify you can use this EAN to send an EDI order where the basket has a branchcode set
Comment 8 Colin Campbell 2018-09-27 11:49:37 UTC
(In reply to Kyle M Hall from comment #6)
> Created attachment 79475 [details] [review] [review]
> Bug 21417: Fix the warning lines to output the basketno

just submitted patch against bug 21425
Comment 9 Colin Campbell 2018-09-27 15:06:14 UTC
Created attachment 79525 [details] [review]
Bug 21417: EDI ordering fails when basket and EAN libraries
Comment 10 Colin Campbell 2018-09-27 15:14:47 UTC
Created attachment 79528 [details] [review]
Supplemental patch to display the null entry

This adds text 'All libraries' to display in the admin screen rather than leaving the column blank to clarify function to user
Comment 11 Pierre-Marc Thibault 2018-09-28 19:50:45 UTC Comment hidden (obsolete)
Comment 12 Pierre-Marc Thibault 2018-09-28 20:11:55 UTC
Created attachment 79607 [details] [review]
Bug 21417 - Display default EAN as such in admin display

A branch value of null is interpreted as the default
Add text 'All libraries' to this case in the display
rather than blank so that its function is clear to the
user

Signed-off-by: Pierre-Marc Thibault <pierre-marc.thibault@inLibro.com>
Comment 13 Martin Renvoize 2018-10-08 13:57:49 UTC
Created attachment 80220 [details] [review]
Bug 21417: EDI ordering fails when basket and EAN libraries do not match

When clicking Create EDIFACT Order on /acqui/basket.pl, an EDIFACT message will not be generated if aqbasket.branch does not match edifact_ean.branchcode. This failure does not generate any sort of error message, it just fails to produce a message.

We should allow ean's to not require a branch to be set, then if we don't find a branch specific each, we can look for the default version of the ean.

Test Plan:
1) Apply this patch set
2) Run updatedatabase
3) Verify you can create a Library EAN without setting a branchcode for it
4) Verify you can use this EAN to send an EDI order where the basket has a branchcode set

Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>
Signed-off-by: Pierre-Marc Thibault <pierre-marc.thibault@inLibro.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2018-10-08 13:57:53 UTC
Created attachment 80221 [details] [review]
Bug 21417: (QA follow-up) Display default EAN as such in admin display

A branch value of null is interpreted as the default
Add text 'All libraries' to this case in the display
rather than blank so that its function is clear to the
user

Signed-off-by: Pierre-Marc Thibault <pierre-marc.thibault@inLibro.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2018-10-08 14:04:07 UTC
I'm a little disturbed by the lack of tests for Koha::EDI, but can't warrant failing qa here for that as it's the entire module that's lacking, not just this bugfix.
Comment 16 Nick Clemens 2018-10-09 16:24:09 UTC
Awesome work all!

Pushed to master for 18.11
Comment 17 Martin Renvoize 2018-10-15 11:54:08 UTC
Pushed to 18.05.x for 18.05.05
Comment 18 Fridolin Somers 2018-11-08 15:31:58 UTC
Pushed to 17.11.x for 17.11.12