Bug 16739 - Generate EDIFACT on basket groups falsely showing when configuration is incomplete
Summary: Generate EDIFACT on basket groups falsely showing when configuration is incom...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jon Knight
QA Contact: Colin Campbell
URL: cgi-bin/koha/acqui/basketgroup.pl?boo...
Keywords:
: 16740 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-06-14 19:22 UTC by Nicole C. Engard
Modified: 2019-10-14 19:56 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 16739: generate edifact showing on basket groups (8.05 KB, patch)
2018-05-08 16:41 UTC, Jon Knight
Details | Diff | Splinter Review
Bug 16739: generate edifact showing on basket groups (8.15 KB, patch)
2018-05-14 15:32 UTC, Jon Knight
Details | Diff | Splinter Review
screencaptrure (244.22 KB, image/jpeg)
2018-09-20 13:17 UTC, Spencer
Details
Bug 16739: generate edifact showing on basket groups (9.54 KB, patch)
2018-09-27 07:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 16739: Show note instead of 500 error for 'Generate EDIFACT' when no EDI account is configured (9.65 KB, patch)
2018-09-27 11:49 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2016-06-14 19:22:46 UTC
When looking at basketgroups the 'generate edifact order' button shows even if edi isn't set up for that vendor and leads to a 500 error page
Comment 1 Marc Véron 2016-07-06 17:41:14 UTC
Duplicate of 6740?
Comment 2 Marc Véron 2016-07-06 17:44:26 UTC
Sorry, I mean 
Bug 16740 - EDIFACT on Basket Groups leads to 500 error (
Comment 3 Katrin Fischer 2016-08-14 19:13:57 UTC
*** Bug 16740 has been marked as a duplicate of this bug. ***
Comment 4 Katrin Fischer 2016-08-14 19:14:36 UTC
Still valid.
Comment 5 Katrin Fischer 2016-08-14 19:15:02 UTC
Still valid.
Comment 6 Jon Knight 2018-05-08 16:41:23 UTC
Created attachment 75173 [details] [review]
Bug 16739: generate edifact showing on basket groups

When a book seller (vendor) does not have EDI account information configured
the basketgroup display still shows a button to generate EDIFACT output which
results in 500 error when clicked.  This patch fixes two aspects of this:

a) it stops the button being displayed in the first place, replacing it with
a message that explains there is not EDIFACT configuration for the vendor.

b) if, somehow, an edifact print operation is passed to the basketgroup
script it detects the lack of an EAN and redirects back to the basket group
display page with a warning message.

To test:
1) Create a vendor with no EDI details.
2) Create a basket with some items in, then close it and add it to a basket
   group for this vendor.
3) Go to that vendor's basket groups, click on the "Closed" tab and then
   click on the 'generate edifact order' button. You should get a 500 error.
4) Apply this patch.
5) Repeat 3, except this time you should find that the 'generate edifact
   order' button has been replaced with a note that there is
   "No EDIFACT configuration for <vendor>".
Comment 7 Owen Leonard 2018-05-10 14:38:07 UTC
This looks like it's working correctly, but the QA tool has several complaints about tab indentation. Indentation should always be done with 4 spaces.

There is one part of the template which I don't understand, starting at line 149:

[% IF (ediaccount) %]
    <div class="btn-group"><a href="[% script_name %]?op=ediprint&amp;basketgroupid=[% basketgroupid %]&amp;booksellerid=[% booksellerid %]" class="btn btn-default btn-sm" id="printbutton"><i class="fa fa-download"></i> Generate EDIFACT order</a></div>
[% ELSE %]
    <div class="btn-group"><a href="[% script_name %]?op=ediprint&amp;basketgroupid=[% basketgroupid %]&amp;booksellerid=[% booksellerid %]" class="btn btn-default btn-sm" id="printbutton"></div>
[% END %]

What is the purpose of the [% ELSE %] section? The <a> tag is incomplete, so I would expect this to break HTML rendering somehow.
Comment 8 Jon Knight 2018-05-10 16:01:15 UTC
(In reply to Owen Leonard from comment #7)
> This looks like it's working correctly, but the QA tool has several
> complaints about tab indentation. Indentation should always be done with 4
> spaces.
> 
> There is one part of the template which I don't understand, starting at line
> 149:
> 
> [% IF (ediaccount) %]
>     <div class="btn-group"><a href="[% script_name
> %]?op=ediprint&amp;basketgroupid=[% basketgroupid %]&amp;booksellerid=[%
> booksellerid %]" class="btn btn-default btn-sm" id="printbutton"><i
> class="fa fa-download"></i> Generate EDIFACT order</a></div>
> [% ELSE %]
>     <div class="btn-group"><a href="[% script_name
> %]?op=ediprint&amp;basketgroupid=[% basketgroupid %]&amp;booksellerid=[%
> booksellerid %]" class="btn btn-default btn-sm" id="printbutton"></div>
> [% END %]
> 
> What is the purpose of the [% ELSE %] section? The <a> tag is incomplete, so
> I would expect this to break HTML rendering somehow.

Ah, good spot - I was putting in other text whilst testing the patch and left the rump in there (but it didn't display on the browser so I didn't notice it).  I'll get rid of the else clause (and see if I can munge the indentation into something more acceptable!).
Comment 9 Jon Knight 2018-05-14 15:32:33 UTC
Created attachment 75316 [details] [review]
Bug 16739: generate edifact showing on basket groups

When a book seller (vendor) does not have EDI account information configured
the basketgroup display still shows a button to generate EDIFACT output which
results in 500 error when clicked.  This patch fixes two aspects of this:

a) it stops the button being displayed in the first place, replacing it with
a message that explains there is not EDIFACT configuration for the vendor.

b) if, somehow, an edifact print operation is passed to the basketgroup
script it detects the lack of an EAN and redirects back to the basket group
display page with a warning message.

To test:
1) Create a vendor with no EDI details.
2) Create a basket with some items in, then close it and add it to a basket
   group for this vendor.
   3) Go to that vendor's basket groups, click on the "Closed" tab and then
      click on the 'generate edifact order' button. You should get a 500 error.
   4) Apply this patch.
   5) Repeat 3, except this time you should find that the 'generate edifact
      order' button has been replaced with a note that there is
      "No EDIFACT configuration for <vendor>".
Comment 10 Spencer 2018-09-20 13:17:46 UTC
Created attachment 79174 [details]
screencaptrure

It appears this patch does not apply.
Comment 11 Martin Renvoize 2018-09-27 07:45:57 UTC
Created attachment 79466 [details] [review]
Bug 16739: generate edifact showing on basket groups

When a book seller (vendor) does not have EDI account information configured
the basketgroup display still shows a button to generate EDIFACT output which
results in 500 error when clicked.  This patch fixes two aspects of this:

a) it stops the button being displayed in the first place, replacing it with
a message that explains there is not EDIFACT configuration for the vendor.

b) if, somehow, an edifact print operation is passed to the basketgroup
script it detects the lack of an EAN and redirects back to the basket group
display page with a warning message.

To test:
1) Create a vendor with no EDI details.
2) Create a basket with some items in, then close it and add it to a basket
   group for this vendor.
   3) Go to that vendor's basket groups, click on the "Closed" tab and then
      click on the 'generate edifact order' button. You should get a 500 error.
   4) Apply this patch.
   5) Repeat 3, except this time you should find that the 'generate edifact
      order' button has been replaced with a note that there is
      "No EDIFACT configuration for <vendor>".

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2018-09-27 07:46:53 UTC
Patch rescued (just needed the filters adding for the xss fixes), and signed off. 

Thanks Jon
Comment 13 Katrin Fischer 2018-09-27 11:49:46 UTC
Created attachment 79484 [details] [review]
Bug 16739: Show note instead of 500 error for 'Generate EDIFACT' when no EDI account is configured

When a book seller (vendor) does not have EDI account information configured
the basketgroup display still shows a button to generate EDIFACT output which
results in 500 error when clicked.  This patch fixes two aspects of this:

a) it stops the button being displayed in the first place, replacing it with
a message that explains there is not EDIFACT configuration for the vendor.

b) if, somehow, an edifact print operation is passed to the basketgroup
script it detects the lack of an EAN and redirects back to the basket group
display page with a warning message.

To test:
1) Create a vendor with no EDI details.
2) Create a basket with some items in, then close it and add it to a basket
   group for this vendor.
   3) Go to that vendor's basket groups, click on the "Closed" tab and then
      click on the 'generate edifact order' button. You should get a 500 error.
   4) Apply this patch.
   5) Repeat 3, except this time you should find that the 'generate edifact
      order' button has been replaced with a note that there is
      "No EDIFACT configuration for <vendor>".

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 14 Katrin Fischer 2018-09-27 11:52:22 UTC
This is a big improvement over the 500 error, so PQA!

While testing I was wondering - maybe we should not show this info at all, when there is no configuration assuming the library is not using EDIFACT at all? Might be a broader discussion to be had - see also bug 16258
Comment 15 Nick Clemens 2018-09-28 19:55:55 UTC
Awesome work all!

Pushed to master for 18.11
Comment 16 Martin Renvoize 2018-10-03 09:27:16 UTC
Pushed to 18.05.x for 18.05.05
Comment 17 Fridolin Somers 2018-10-10 09:53:07 UTC
Pushed to 17.11.x for 17.11.11