To reproduce: 1) Install FreeForm and enable ILLModule, run: bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh) 2) Configure ILLModuleDisclaimerByType, visit: /cgi-bin/koha/admin/preferences.pl?tab=&op=search&searchfield=ILLModuleDisclaimerByType Click "edit" and paste the example code into the sys pref value 3) Create a new OPAC ILL request, visit: /cgi-bin/koha/opac-illrequests.pl?method=create&backend=FreeForm 4) Dont put a type or branch, click 'Create', on the next screen click 'Submit' 5) Notice it explodes 6) Apply patch and repeat. It no longer explodes and now shows an error message.
Created attachment 160456 [details] [review] Bug 35685: Fix after creation actions Test plan: 1) Install FreeForm and enable ILLModule, run: bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh) 2) Configure ILLModuleDisclaimerByType, visit: /cgi-bin/koha/admin/preferences.pl?tab=&op=search&searchfield=ILLModuleDisclaimerByType Click "edit" and paste the example code into the sys pref value 3) Create a new OPAC ILL request, visit: /cgi-bin/koha/opac-illrequests.pl?method=create&backend=FreeForm 4) Dont put a type or branch, click 'Create', on the next screen click 'Submit' 5) Notice it explodes 6) Apply patch and repeat. It no longer explodes and now shows an error message.
Created attachment 160563 [details] [review] Bug 35685: Fix after creation actions Test plan: 1) Install FreeForm and enable ILLModule, run: bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh) 2) Configure ILLModuleDisclaimerByType, visit: /cgi-bin/koha/admin/preferences.pl?tab=&op=search&searchfield=ILLModuleDisclaimerByType Click "edit" and paste the example code into the sys pref value 3) Create a new OPAC ILL request, visit: /cgi-bin/koha/opac-illrequests.pl?method=create&backend=FreeForm 4) Dont put a type or branch, click 'Create', on the next screen click 'Submit' 5) Notice it explodes 6) Apply patch and repeat. It no longer explodes and now shows an error message. Signed-off-by: David Nind <david@davidnind.com>
This does fix the issue, and I have signed off. However, some comments - not sure how hard these are to do or whether it depends on the ILL plugin (I'm assuming this is the case): 1. If the "Destination library" is not selected, then the message at the top says: "Please Note: Branch is a mandatory field.". In my view the error should match the UI, that is, instead of "Branch" it should say "Destination library is required" 2. The normal UI for forms in the OPAC (from what I can see) is to have "Required" under the field name, and then a message if mandatory fields are not completed. I assume that this depends on how the ILL plugin works/coded though. Testing notes (using KTD): 1. Make the ILL request using the OPAC! . Go to the OPAC . Log in as a patron . Make the request using the "Interlibrary loan requests" tab/section
(In reply to David Nind from comment #3) > This does fix the issue, and I have signed off. > > However, some comments - not sure how hard these are to do or whether it > depends on the ILL plugin (I'm assuming this is the case): > > 1. If the "Destination library" is not selected, then the message at the top > says: "Please Note: Branch is a mandatory field.". In my view the error > should match the UI, that is, instead of "Branch" it should say "Destination > library is required" > > 2. The normal UI for forms in the OPAC (from what I can see) is to have > "Required" under the field name, and then a message if mandatory fields are > not completed. I assume that this depends on how the ILL plugin works/coded > though. Hi David, thanks for testing! Your assumptions are correct. The form and its behavior is dependent on the specific ILL backend which the request is being created to, in this example it's FreeForm. In bug 35570 I'm submitting work to add FreeForm to Koha core, to become a backend available out of the box instead of it being required to install an external one in order to make use of the ILL module. I mention bug 35570 because once that's gone through, these sort of enhancements can be worked directly into Koha, and have at least the Koha native backend be consistent with everything else.
(In reply to Pedro Amorim from comment #4) ... > Hi David, thanks for testing! > Your assumptions are correct. The form and its behavior is dependent on the > specific ILL backend which the request is being created to, in this example > it's FreeForm. > > In bug 35570 I'm submitting work to add FreeForm to Koha core, to become a > backend available out of the box instead of it being required to install an > external one in order to make use of the ILL module. > I mention bug 35570 because once that's gone through, these sort of > enhancements can be worked directly into Koha, and have at least the Koha > native backend be consistent with everything else. Thanks Pedro! I'll look at signing that one off over the next few days. I'll keep an eye out for other ILL bugs.
print $query->redirect('/cgi-bin/koha/opac-illrequests.pl?message=2'); # After creation actions if ( $params->{type_disclaimer_submitted} ) { $type_disclaimer->after_request_created( $params, $request ); print $query->redirect('/cgi-bin/koha/opac-illrequests.pl?message=2'); Possibly, two redirects in a row ?
Created attachment 161503 [details] [review] Bug 35685: Fix after creation actions Test plan: 1) Install FreeForm and enable ILLModule, run: bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh) 2) Configure ILLModuleDisclaimerByType, visit: /cgi-bin/koha/admin/preferences.pl?tab=&op=search&searchfield=ILLModuleDisclaimerByType Click "edit" and paste the example code into the sys pref value 3) Create a new OPAC ILL request, visit: /cgi-bin/koha/opac-illrequests.pl?method=create&backend=FreeForm 4) Dont put a type or branch, click 'Create', on the next screen click 'Submit' 5) Notice it explodes 6) Apply patch and repeat. It no longer explodes and now shows an error message. 7) (After FQA): Create a normal request, input type and library, submit the type disclaimer 8) Run: echo "select * from illrequestattributes;" | koha-mysql kohadev 9) Notice type_disclaimer_date and type_disclaimer_value are correctly saved Signed-off-by: David Nind <david@davidnind.com>
(In reply to Marcel de Rooy from comment #6) > print > $query->redirect('/cgi-bin/koha/opac-illrequests.pl?message=2'); > > # After creation actions > if ( $params->{type_disclaimer_submitted} ) { > $type_disclaimer->after_request_created( $params, > $request ); > print > $query->redirect('/cgi-bin/koha/opac-illrequests.pl?message=2'); > > > Possibly, two redirects in a row ? Thank you Marcel, certainly not great code! Thanks for pointing it out. I've amended the original patch and updated the test plan to reinforce that there are no regressions in how the data is saved.
Created attachment 164457 [details] [review] Bug 35685: Fix after creation actions Test plan: 1) Install FreeForm and enable ILLModule, run: bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh) 2) Configure ILLModuleDisclaimerByType, visit: /cgi-bin/koha/admin/preferences.pl?tab=&op=search&searchfield=ILLModuleDisclaimerByType Click "edit" and paste the example code into the sys pref value 3) Create a new OPAC ILL request, visit: /cgi-bin/koha/opac-illrequests.pl?method=create&backend=FreeForm 4) Dont put a type or branch, click 'Create', on the next screen click 'Submit' 5) Notice it explodes 6) Apply patch and repeat. It no longer explodes and now shows an error message. 7) (After FQA): Create a normal request, input type and library, submit the type disclaimer 8) Run: echo "select * from illrequestattributes;" | koha-mysql kohadev 9) Notice type_disclaimer_date and type_disclaimer_value are correctly saved Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Pushed for 24.05! Well done everyone, thank you!
Pushed to 23.11.x for 23.11.06
Missing 23.05.x dependencies, no backport.