|
Description
Andrew Fuerste-Henry
2018-09-26 16:30:18 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.
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. 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. 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. 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. Created attachment 79475 [details] [review] Bug 21417: Fix the warning lines to output the basketno 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 (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 Created attachment 79525 [details] [review] Bug 21417: EDI ordering fails when basket and EAN libraries 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 (In reply to Colin Campbell from comment #10) > Created attachment 79528 [details] [review] [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 Can you provide a test plan to see the change easily please? The branchcode column cannot be NULL and you have no choice to assign a library to an EAN, so it's a bit hard to test it. 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> 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> 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> 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. Awesome work all! Pushed to master for 18.11 Pushed to 18.05.x for 18.05.05 Pushed to 17.11.x for 17.11.12 |