Description
Nicole C. Engard
2016-06-14 19:22:46 UTC
Duplicate of 6740? Sorry, I mean Bug 16740 - EDIFACT on Basket Groups leads to 500 error ( *** Bug 16740 has been marked as a duplicate of this bug. *** Still valid. Still valid. 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>". 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&basketgroupid=[% basketgroupid %]&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&basketgroupid=[% basketgroupid %]&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. (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&basketgroupid=[% basketgroupid %]&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&basketgroupid=[% basketgroupid %]&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!). 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>". Created attachment 79174 [details]
screencaptrure
It appears this patch does not apply.
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> Patch rescued (just needed the filters adding for the xss fixes), and signed off. Thanks Jon 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> 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 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.11 |