Summary: | Having a single EDI EAN account produces a bad redirect | ||
---|---|---|---|
Product: | Koha | Reporter: | Lucas Gass (lukeg) <lucas> |
Component: | Acquisitions | Assignee: | Martin Renvoize (ashimema) <martin.renvoize> |
Status: | RESOLVED FIXED | QA Contact: | Nick Clemens (kidclamp) <nick> |
Severity: | major | ||
Priority: | P5 - low | CC: | alexbuckley, chughesman, david, martin.renvoize, matt.blenkinsop, nick, nugged, phil, victor, wainuiwitikapark |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
GIT URL: | Change sponsored?: | --- | |
Patch complexity: | --- | Documentation contact: | |
Documentation submission: | Text to go in the release notes: |
This fixes creating an EDIFACT order for a basket in acquisitions - if there was only one library EAN defined, then a 403 page error was generated. It also simplifies creating an EDIFACT order:
- If there are no library EANs defined, the "Create EDIFACT order" button is greyed out and has a tooltip "You must define an EAN in Administration > Library EANs".
- If there is only one library EAN defined, you are prompted to generate the order without needing to select an EAN.
- If there is more than one library EAN, the "Create EDIFACT order" button incorporates a dropdown list with the available library EANs.
(The error is related to the CSRF changes added in Koha 24.05 to improve form security.)
|
|
Version(s) released in: |
25.05.00,24.11.02
|
Circulation function: | |
Bug Depends on: | 39787, 36192 | ||
Bug Blocks: | 39572 | ||
Attachments: |
Bug 37993: Replace edi_ean link with dropdown
Bug 37993: Remove edi_ean page Bug 37993: Replace edi_ean link with dropdown Bug 37993: Remove edi_ean page Bug 37993: Replace edi_ean link with dropdown Bug 37993: Remove edi_ean page Bug 37993: (QA follow-up): Add missing data for breadcrumbs Bug 37993: (QA follow-up) Use submit form link asset guideline JS16 Bug 37993: (QA follow-up) Add a hint/warning when no EANs are defined Bug 37993: Replace edi_ean link with dropdown Bug 37993: Remove edi_ean page Bug 37993: (QA follow-up): Add missing data for breadcrumbs Bug 37993: (QA follow-up) Use submit form link asset guideline JS16 Bug 37993: (QA follow-up) Add a hint/warning when no EANs are defined Bug 37993: (QA follow-up) Fix Typo Bug 37993: (QA follow-up) Fix tooltip and disabled display |
Description
Lucas Gass (lukeg)
2024-09-23 21:44:46 UTC
*** 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! All good here. Nice work everyone! Pushed to 24.11.x for 24.11.01 Hi Martin, Just testing this for 24.05.x backport. Cherry-picking the second patch from 24.11.x onto 24.05.x conflicts (see below). Would you mind please attaching rebased 24.05.x patches to this bug report? ---- kohadev-koha@kohadevbox:koha(24.05.x)$ git cherry-pick 4dcaef65ca9595663fcc0ba6e4b6208eb92ac893 CONFLICT (modify/delete): koha-tmpl/intranet-tmpl/prog/en/modules/acqui/edi_ean.tt deleted in 4dcaef65ca9 (Bug 37993: Remove edi_ean page) and modified in HEAD. Version HEAD of koha-tmpl/intranet-tmpl/prog/en/modules/acqui/edi_ean.tt left in tree. error: could not apply 4dcaef65ca9... Bug 37993: Remove edi_ean page hint: After resolving the conflicts, mark them with hint: "git add/rm <pathspec>", then run hint: "git cherry-pick --continue". hint: You can instead skip this commit with "git cherry-pick --skip". hint: To abort and get back to the state before "git cherry-pick", hint: run "git cherry-pick --abort". kohadev-koha@kohadevbox:koha(24.05.x|CHERRY-PICKING)$ git status On branch 24.05.x You are currently cherry-picking commit 4dcaef65ca9. (fix conflicts and run "git cherry-pick --continue") (use "git cherry-pick --skip" to skip this patch) (use "git cherry-pick --abort" to cancel the cherry-pick operation) Changes to be committed: deleted: acqui/edi_ean.pl Unmerged paths: (use "git add/rm <file>..." as appropriate to mark resolution) deleted by them: koha-tmpl/intranet-tmpl/prog/en/modules/acqui/edi_ean.tt kohadev-koha@kohadevbox:koha(24.05.x|CHERRY-PICKING)$ ---- Many thanks in advance. Not backporting to 24.05.x unless requested Consider there is Bug 39787, caused by current one. |