*** Bug 38401 has been marked as a duplicate of this bug. *** Minimal test plan: 1. Enable the EDIFACT syspref 2. Add a Library EAN: Administration > Library EANs 3. Add an EDI account for a vendor: Administration > EDI accounts 4. Go to a basket 5. click "Create EDIFACT order" 6. Error 403 Programming error - op 'cud-ediorder' must not start with 'cud-' for GET http://localhost:8081/intranet/acqui/basket.pl?basketno=1&op=cud-ediorder&ean=11111 (referer: http://localhost:8081/cgi-bin/koha/acqui/basket.pl?basketno=1) Created attachment 175024 [details] [review] Bug 37993: Replace edi_ean link with dropdown The edi_ean page is a slim page with a single select input for selecting the appropriate library and ean. This patch replaces the link to that page with a simple select in the menu on the original page instead. Created attachment 175025 [details] [review] Bug 37993: Remove edi_ean page In the previous patch we replace the only link to the edi_ean selection page with a select list embedded in the page. As such, we no longer need this page at all. Created attachment 175026 [details] [review] Bug 37993: Replace edi_ean link with dropdown The edi_ean page is a slim page with a single select input for selecting the appropriate library and ean. This patch replaces the link to that page with a simple select in the menu on the original page instead. Signed-off-by: David Nind <david@davidnind.com> Created attachment 175027 [details] [review] Bug 37993: Remove edi_ean page In the previous patch we replace the only link to the edi_ean selection page with a select list embedded in the page. As such, we no longer need this page at all. Signed-off-by: David Nind <david@davidnind.com> Created attachment 175042 [details] [review] Bug 37993: Replace edi_ean link with dropdown The edi_ean page is a slim page with a single select input for selecting the appropriate library and ean. This patch replaces the link to that page with a simple select in the menu on the original page instead. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 175043 [details] [review] Bug 37993: Remove edi_ean page In the previous patch we replace the only link to the edi_ean selection page with a select list embedded in the page. As such, we no longer need this page at all. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 175044 [details] [review] Bug 37993: (QA follow-up): Add missing data for breadcrumbs The vendor name was missing from the breadcrumbs, this patch adds it back in Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Small fix added to address missing data in the breadcrumbs but otherwise the QA script is happy and all works as advertised Testing again to add another signoff to be less out of bounds regarding to: > If a patch is not passed QA for one month, then someone from the same company > or institution as the patch author can QA it, as long as the patch signer is > from another company or institution. The RM can still ask for a second > sign-off in this case. https://wiki.koha-community.org/wiki/Development_workflow#Rules ----- It works! :) I don't know how to actually test EDIFACT to confirm a valid ean is picked and that it will work irl but here is a circumvention: Compare the POST request which leads to the page "Are you sure you want to generate an EDIFACT order and close basket My Basket?" With and without the patch, we end up with the same params so the rest of the workflow should continue working :) ----- Looking at the code, there is a path for [% IF eans.count == 1 %] but it somehow doesn't get executed when I have only one ean. I still have the dropdown and it has one element :o [% eans.count %] doesn't display anything. With one or two eans. ---- When having no Library EAN the dropdown is there but it's empty. Which doesn't look great UI wise. And UX wise it makes the process less discoverable than the old page that said: > Select the library account submitting the EDI order > Select ordering library account: Which was a hint (not great but still a hint) that one should go to "Library EANs" page and create an EAN. Created attachment 175069 [details] [review] Bug 37993: (QA follow-up) Use submit form link asset guideline JS16 Created attachment 175070 [details] [review] Bug 37993: (QA follow-up) Add a hint/warning when no EANs are defined Thanks for the follow-ups here guys, nice bit of golfing and some solid improvements made. I'll properly review the follow-ups and add my own stamp back and then reset to PQA.. thanks again. Created attachment 175072 [details] [review] Bug 37993: Replace edi_ean link with dropdown The edi_ean page is a slim page with a single select input for selecting the appropriate library and ean. This patch replaces the link to that page with a simple select in the menu on the original page instead. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 175073 [details] [review] Bug 37993: Remove edi_ean page In the previous patch we replace the only link to the edi_ean selection page with a select list embedded in the page. As such, we no longer need this page at all. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 175074 [details] [review] Bug 37993: (QA follow-up): Add missing data for breadcrumbs The vendor name was missing from the breadcrumbs, this patch adds it back in Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 175075 [details] [review] Bug 37993: (QA follow-up) Use submit form link asset guideline JS16 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 175076 [details] [review] Bug 37993: (QA follow-up) Add a hint/warning when no EANs are defined Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> OK acqui/basket.pl FAIL koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt FAIL spelling diasbled ==> disabled QA tools complain, but fixing this typo might have an unwanted effect? Please verify! <a title="You must define an EAN in Administration -> Library EANs" class="btn btn-default btn-xs" diasbled="disabled" href="#"><i class="fa fa-download"></i> Create EDIFACT order</a> (In reply to Katrin Fischer from comment #20) > OK acqui/basket.pl > > FAIL koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt > FAIL spelling > diasbled ==> disabled > > > QA tools complain, but fixing this typo might have an unwanted effect? > Please verify! > > <a title="You must define an EAN in Administration -> Library EANs" > class="btn btn-default btn-xs" diasbled="disabled" href="#"><i class="fa > fa-download"></i> Create EDIFACT order</a> Nah, it's just a typo.. needs fixing. Created attachment 175081 [details] [review] Bug 37993: (QA follow-up) Fix Typo (In reply to Martin Renvoize (ashimema) from comment #21) > (In reply to Katrin Fischer from comment #20) > > OK acqui/basket.pl > > > > FAIL koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt > > FAIL spelling > > diasbled ==> disabled > > > > > > QA tools complain, but fixing this typo might have an unwanted effect? > > Please verify! > > > > <a title="You must define an EAN in Administration -> Library EANs" > > class="btn btn-default btn-xs" diasbled="disabled" href="#"><i class="fa > > fa-download"></i> Create EDIFACT order</a> > > Nah, it's just a typo.. needs fixing. OK, was worried it would make the "disabled" work when it wasn't before blocking something. Pushed for 25.05! Well done everyone, thank you! Thanks all, nice team work :D ----- Does anyone else when having no library EAN account doesn't see the tooltip on the "create EDIFACT order" button when clicking on it? Clicking prevents the tooltip from appearing on Firefox and Chromium. As a result when trying to use the button, I almost never see the tooltip. I have to voluntarily over on it. So basically know that there is a tooltip there. Which mean knowing in advance the issue the tooltip is warning about. Created attachment 175132 [details] [review] Bug 37993: (QA follow-up) Fix tooltip and disabled display This patch re-arranges the button markup slightly to properly style the disabled 'Create EDI Order' button when there are no EAN's defined for the library. It also moves the tooltip to the container btn-group element to ensure the tooltip works even though the button itself is disabled. Pushed for 25.05! Well done everyone, thank you! |
In acqui/edi_ean.pl: if ( @eans == 1 ) { my $ean = $eans[0]->ean; print $query->redirect( "/cgi-bin/koha/acqui/basket.pl?basketno=$basketno&op=cud-ediorder&ean=$ean" ); } So if you only have the single 'Library EAN' account Koha redirects to: /cgi-bin/koha/acqui/basket.pl?basketno=$basketno&op=cud-ediorder&ean=$ean This causes an error "op 'cud-ediorder' must not start with cud-"